Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: secure allowance creationby including protocol information #2930

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

pavanjoshi914
Copy link
Contributor

Describe the changes you have made in this PR

Include the fully qualified origin for allowances
Migrate existing allowances (prefix with https)
Potentially display a warning on the enable screen when connecting to non-ssl domains (MITM)

Link this PR to an issue [optional]

Fixes #2437

Type of change

(Remove other not matching type)

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

image

How has this been tested?

create allowance on http site

Checklist

  • Self-review of changed code
  • Manual testing
  • Added automated tests where applicable
  • Update Docs & Guides
  • For UI-related changes
  • Darkmode
  • Responsive layout

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we save new allowances, don't we there also need a change to save it with the protocol?

@pavanjoshi914
Copy link
Contributor Author

when we save new allowances, don't we there also need a change to save it with the protocol?

is there any other way we can create allowances? if we use providers to create one. we already store allowance host along with https prefix

@bumi
Copy link
Collaborator

bumi commented Dec 18, 2023

@pavanjoshi914
Copy link
Contributor Author

what do we use in here? master/src/extension/background-script/actions/allowances/add.ts#L5

aah! yes you are right, we add allowances while confirmPayment + doing keysends as well. fixed

@reneaaron reneaaron added the next-release To be included in the next release label Dec 18, 2023
@bumi
Copy link
Collaborator

bumi commented Dec 18, 2023

This currently breaks the links on the connected sites pages.

I think we should only add the warning and not change the allowance and the host usage for now. We can do this in a follow-up

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's only add the warning. the change in the allowance has some issues like for example the links to the site a broken.

let's do this in a follow-up PR.

@reneaaron reneaaron removed the next-release To be included in the next release label Dec 18, 2023
@pavanjoshi914
Copy link
Contributor Author

e change in the allowance has some issues like for example the links to the site a broken.

resolved! + for existing connected sites when we have migrations running. they will not cause the problem

@pavanjoshi914 pavanjoshi914 requested a review from bumi December 29, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for protocol and port in allowances
3 participants