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

fix(solo): don't enforce origin identity; we have access tokens #3838

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

michaelfig
Copy link
Member

While checking that the connections to our private parts (such as
the privileged CapTP) matched "localhost" and a few others was a
stopgap before we had an accessToken (capability), now it is
really a pain that impedes (but does not prevent) more general
access when a user wants it.

I believe it is now safe to remove this origin check, especially
since it can trivially be bypassed, it just is a nuisance.

@michaelfig michaelfig added security solo the solo node (packages/solo) labels Sep 16, 2021
@michaelfig michaelfig self-assigned this Sep 16, 2021
@michaelfig
Copy link
Member Author

michaelfig commented Sep 16, 2021

@dckc, please try this PR against your gitpod stuff. It should solve #7032 's problems with connecting from a non-localhost Origin. That check was a kludge before we had accessTokens.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Please address the TODO about constant time compare in verifyToken as suggested below.

And updated outdated comment and function nam.e

packages/solo/src/web.js Show resolved Hide resolved
packages/solo/src/web.js Show resolved Hide resolved
packages/solo/src/web.js Show resolved Hide resolved
@dckc
Copy link
Member

dckc commented Sep 16, 2021

yes, it does address the gitpod vs localhost issue. Thanks!

The URL surgery to take the localhost URL printed by agoric open and combine it with the right origin is tedious, but that's a separable issue, and not clearly one that's in-scope for agoric-sdk.

While checking that the connections to our private parts (such as
the privileged CapTP) matched "localhost" and a few others was a
stopgap before we had an accessToken (capability), now it is
really a pain that impedes (but does not prevent) more general
access when a user wants it.

I believe it is now safe to remove this origin check, especially
since it can trivially be bypassed, it just is a nuisance.
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

👍

@michaelfig michaelfig merged commit 2e9b4b8 into master Sep 17, 2021
@michaelfig michaelfig deleted the mfig-ignore-solo-web-origin branch September 17, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security solo the solo node (packages/solo)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants