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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c9c37a
Auth: Add input validation for instance URL
abeatrix Feb 13, 2024
8410754
clean up
abeatrix Feb 13, 2024
7711472
update regex
abeatrix Feb 13, 2024
dc3e994
improve URL validation
abeatrix Feb 13, 2024
075b903
update local storage provider
abeatrix Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions vscode/src/services/AuthMenus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as vscode from 'vscode'

import { isDotCom, LOCAL_APP_URL } from '@sourcegraph/cody-shared'
import { isSourcegraphToken } from './AuthProvider'

interface LoginMenuItem {
id: string
Expand Down Expand Up @@ -60,6 +61,13 @@ export async function showInstanceURLInputBox(title: string): Promise<string | u
placeHolder: 'https://sourcegraph.example.com',
password: false,
ignoreFocusOut: true,
// valide input to ensure the user is not entering a token as URL
validateInput: (value: string) => {
if (isSourcegraphToken(value)) {
return 'Invalid input. Please enter a valid URL'
}
return null
},
})

if (typeof result === 'string') {
Expand Down
34 changes: 23 additions & 11 deletions vscode/src/services/AuthProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]+/
abeatrix marked this conversation as resolved.
Show resolved Hide resolved
Copy link

@dcomas dcomas Feb 13, 2024 •
edited
Loading

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?

Copy link

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

Copy link

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 •
edited
Loading

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:

cody/vscode/src/services/AuthProvider.ts

Lines 463 to 465 in 7711472

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

export const isSourcegraphToken = (text: string): boolean => sourcegraphTokenRegex.test(text)

export class AuthProvider {
private endpointHistory: string[] = []

Expand Down Expand Up @@ -447,21 +450,30 @@ export function isNetworkError(error: Error): boolean {
}

function formatURL(uri: string): string | null {
if (!uri) {
return null
}
// Check if the URI is in the correct URL format
// Add missing https:// if needed
if (!uri.startsWith('http')) {
uri = `https://${uri}`
}
try {
if (!uri) {
return null
}

// Check if the URI is a sourcegraph token
if (isSourcegraphToken(uri)) {
const errorMsg = 'Token is not a valid URL'
void vscode.window.showErrorMessage(errorMsg)
throw new Error(errorMsg)
}

// Check if the URI is in the correct URL format
// Add missing https:// if needed
if (!uri.startsWith('http')) {
uri = `https://${uri}`
}

const endpointUri = new URL(uri)
return endpointUri.href
} catch {
console.error('Invalid URL')
} catch (error) {
console.error('Invalid URL: ', error)
return null
}
return null
}

async function showAuthResultMessage(
Expand Down
7 changes: 7 additions & 0 deletions vscode/src/services/LocalStorageProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { Memento } from 'vscode'
import type { ChatHistory, ChatInputHistory, UserLocalHistory } from '@sourcegraph/cody-shared'

import type { AuthStatus } from '../chat/protocol'
import { isSourcegraphToken } from './AuthProvider'

type ChatHistoryKey = `${string}-${string}`
type AccountKeyedChatHistory = {
Expand Down Expand Up @@ -62,6 +63,12 @@ class LocalStorage {
if (!endpoint) {
return
}
// Do not save sourcegraph tokens as the last used endpoint
// Clear last used endpoint if it is a sourcegraph token
if (isSourcegraphToken(endpoint)) {
this.deleteEndpoint()
return
}
try {
const uri = new URL(endpoint).href
await this.storage.update(this.LAST_USED_ENDPOINT, uri)
Expand Down
Loading