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

Auth: Add input validation for instance URL #3156

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Auth: Add input validation for instance URL #3156

merged 5 commits into from
Feb 13, 2024

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Feb 13, 2024

This commit adds validation for sourcegraph tokens in the AuthMenus and AuthProvider files. The showInstanceURLInputBox function in AuthMenus now checks if the user is entering a token as a URL and displays an error message if so.
Similarly, the formatURL function in AuthProvider now throws an error if the URI is a sourcegraph token, and return null.

Additionally, the LocalStorageProvider file now ignores and clears the last used endpoint if the provided endpoint is a sourcegraph token.

image

Update placeholder value for URL to always starts with https:// to make it clear that the field is for URL input:

image

Test plan

Screen.Recording.2024-02-13.at.8.57.40.AM.mov

@abeatrix abeatrix requested review from a team and removed request for a team February 13, 2024 17:52
@@ -31,6 +31,9 @@ import { telemetryRecorder } from './telemetry-v2'
type Listener = (authStatus: AuthStatus) => void
type Unsubscribe = () => void

const sourcegraphTokenRegex = /sgp_[a-zA-Z0-9]+_[a-zA-Z0-9]+/
Copy link

@dcomas dcomas Feb 13, 2024

Choose a reason for hiding this comment

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

@abeatrix this does not look to be catching the old format. Does everyone uses the new token format only? Can we include the ones from this list? cc:@willdollman @shivasurya

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dcomas @willdollman @shivasurya Thanks for the link! Would it be safe to assume anything without a . in the URL is an invalid URL?

Choose a reason for hiding this comment

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

Broadly no (e.g. http://localhost) and slight concern that some edge case customer will have a crazy dns setup that this would break.

What about ensuring the url matches this: ([.]|^https?:\/\/)

Matches anything containg a dot or anything starting with https?://
Matches:

https://sourcegraph.com
http://localhost
sourcegraph.com
http://customersourcegraph

Doesn't match:

sgp_abcd_foobar
sgp_abcd
sgp_foobar
foobar

Choose a reason for hiding this comment

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

I'm making the assumption that we automatically add the missing https:// if a user enters a url without a prefix (sourcegraph.com)

Copy link
Member

@unknwon unknwon Feb 13, 2024

Choose a reason for hiding this comment

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

(what Will is proposing) LGTM 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm making the assumption that we automatically add the missing https:// if a user enters a url without a prefix (sourcegraph.com)

@willdollman yea i added the check before we add the https:// prefix:

if (!uri.startsWith('http')) {
uri = `https://${uri}`
}

But that's a good callout, I'll make sure to include those cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just updated the regex for catching token:

Sourcegraph Access Token (v3):
image

Sourcegraph Access Token (v2, deprecated):
image

Sourcegraph Access Token (v1, deprecated):
image

Catch tokens with http prefix:

image

image

image

Also updated the check to ensure the URL must starts with http to reduce confusion:
image
image
image

@abeatrix abeatrix requested review from willdollman, dcomas, unknwon and a team February 13, 2024 19:04
@willdollman
Copy link

Validating new tokens lgtm - thanks!

@abeatrix are you able to also validate that if a user already has a token saved in local storage as the endpoint, will this update result in the token being purged? I see it will purge via saveEndpoint but is that called if the endpoint is already stored and just being read?

@abeatrix
Copy link
Contributor Author

Validating new tokens lgtm - thanks!

@abeatrix are you able to also validate that if a user already has a token saved in local storage as the endpoint, will this update result in the token being purged? I see it will purge via saveEndpoint but is that called if the endpoint is already stored and just being read?

@willdollman we call the auth function when we first get the stored endpoint from the local storage, and then first thing we do in the auth function is to run the formatURL function on the endpoint which now will return null if the store endpoint is a token. Does this address your concern?

@willdollman
Copy link

Yep looks good! My concern was whether the bad endpoint is (eventually) removed from local storage and I see in auth() that we call this.storeAuthInfo(endpoint, token) which will overwrite endpoint with the empty string returned by formatURL.

@willdollman we call the auth function when we first get the stored endpoint from the local storage, and then first thing we do in the auth function is to run the formatURL function on the endpoint which now will return null if the store endpoint is a token. Does this address your concern?

Copy link

@willdollman willdollman left a comment

Choose a reason for hiding this comment

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

lgtm. Thank you for the quick work on this! 🙏

@abeatrix abeatrix merged commit 75e2116 into main Feb 13, 2024
14 of 15 checks passed
@abeatrix abeatrix deleted the bee/auth-error branch February 13, 2024 21:06
abeatrix added a commit that referenced this pull request Feb 13, 2024
This commit adds validation for sourcegraph tokens in the AuthMenus and
AuthProvider files. The showInstanceURLInputBox function in AuthMenus
now checks if the user is entering a token as a URL and displays an
error message if so.
Similarly, the formatURL function in AuthProvider now throws an error if
the URI is a sourcegraph token, and return `null`.

Additionally, the LocalStorageProvider file now ignores and clears the
last used endpoint if the provided endpoint is a sourcegraph token.


![image](https://github.com/sourcegraph/cody/assets/68532117/c77005be-edd9-4018-9c42-4655baff2782)

Update placeholder value for URL to always starts with `https://` to
make it clear that the field is for URL input:


![image](https://github.com/sourcegraph/cody/assets/68532117/cea4f78a-d39e-4819-bc97-a59f6af4c369)



## Test plan

<!-- Required. See
https://sourcegraph.com/docs/dev/background-information/testing_principles.
-->




https://github.com/sourcegraph/cody/assets/68532117/9d44427a-3b8b-4c35-812d-aa5b22120d6d
@abeatrix abeatrix mentioned this pull request Feb 13, 2024
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.

None yet

4 participants