Skip to content

fix(sso): login with custom startUrl not allowed #6368

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

Merged
merged 4 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "Auth: Valid StartURL not accepted at login"
}
11 changes: 9 additions & 2 deletions packages/core/src/auth/sso/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,15 @@
export const builderIdStartUrl = 'https://view.awsapps.com/start'
export const internalStartUrl = 'https://amzn.awsapps.com/start'

/**
* Doc: https://docs.aws.amazon.com/singlesignon/latest/userguide/howtochangeURL.html
*/
export const ssoUrlFormatRegex =
/^(https?:\/\/(.+)\.awsapps\.com\/start|https?:\/\/identitycenter\.amazonaws\.com\/ssoins-[\da-zA-Z]{16})\/?$/

export const ssoUrlFormatMessage =
'URLs must start with http:// or https://. Example: https://d-xxxxxxxxxx.awsapps.com/start'
/**
* It is possible for a start url to be a completely custom url that redirects to something that matches the format
* below, so this message is only a warning.
*/
export const ssoUrlFormatMessage = 'URL possibly invalid. Expected format: https://xxxxxxxxxx.awsapps.com/start'
export const urlInvalidFormatMessage = 'URL format invalid. Expected format: https://xxxxxxxxxx.com/yyyy'
5 changes: 5 additions & 0 deletions packages/core/src/login/webview/vue/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { AuthEnabledFeatures, AuthError, AuthFlowState, AuthUiClick, userCancell
import { DevSettings } from '../../../shared/settings'
import { AuthSSOServer } from '../../../auth/sso/server'
import { getLogger } from '../../../shared/logger/logger'
import { isValidUrl } from '../../../shared/utilities/uriUtils'

export abstract class CommonAuthWebview extends VueWebview {
private readonly className = 'CommonAuthWebview'
Expand Down Expand Up @@ -276,4 +277,8 @@ export abstract class CommonAuthWebview extends VueWebview {
cancelAuthFlow() {
AuthSSOServer.lastInstance?.cancelCurrentFlow()
}

validateUrl(url: string) {
return isValidUrl(url)
}
}
56 changes: 46 additions & 10 deletions packages/core/src/login/webview/vue/login.vue
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@
@keydown.enter="handleContinueClick()"
/>
<h4 class="start-url-error">{{ startUrlError }}</h4>
<h4 class="start-url-warning">{{ startUrlWarning }}</h4>
<div class="title topMargin">Region</div>
<div class="hint">AWS Region that hosts identity directory</div>
<select
Expand Down Expand Up @@ -278,7 +279,7 @@ import { LoginOption } from './types'
import { CommonAuthWebview } from './backend'
import { WebviewClientFactory } from '../../../webviews/client'
import { Region } from '../../../shared/regions/endpoints'
import { ssoUrlFormatRegex, ssoUrlFormatMessage } from '../../../auth/sso/constants'
import { ssoUrlFormatRegex, ssoUrlFormatMessage, urlInvalidFormatMessage } from '../../../auth/sso/constants'

const client = WebviewClientFactory.create<CommonAuthWebview>()

Expand Down Expand Up @@ -340,6 +341,7 @@ export default defineComponent({
stage: 'START' as Stage,
regions: [] as Region[],
startUrlError: '',
startUrlWarning: '',
selectedRegion: 'us-east-1',
startUrl: '',
app: this.app,
Expand All @@ -365,7 +367,7 @@ export default defineComponent({

// Pre-select the first available login option
await this.preselectLoginOption()
this.handleUrlInput() // validate the default startUrl
await this.handleUrlInput() // validate the default startUrl
},
methods: {
toggleItemSelection(itemId: number) {
Expand Down Expand Up @@ -475,18 +477,48 @@ export default defineComponent({
this.stage = 'CONNECTED'
}
},
handleUrlInput() {
if (this.startUrl && !ssoUrlFormatRegex.test(this.startUrl)) {
this.startUrlError = ssoUrlFormatMessage
} else if (this.startUrl && this.existingStartUrls.some((url) => url === this.startUrl)) {
this.startUrlError =
'A connection for this start URL already exists. Sign out before creating a new one.'
} else {
this.startUrlError = ''
async handleUrlInput() {
const messages = await resolveStartUrlMessages(this.startUrl, this.existingStartUrls)
this.startUrlError = messages.error
this.startUrlWarning = messages.warning

if (!messages.error && !messages.warning) {
void client.storeMetricMetadata({
credentialStartUrl: this.startUrl,
})
}

async function resolveStartUrlMessages(
startUrl: string | undefined,
existingStartUrls: string[]
): Promise<{ warning: string; error: string }> {
// No URL
if (!startUrl) {
return { error: '', warning: '' }
}

// Validate URL format
if (!ssoUrlFormatRegex.test(startUrl)) {
console.log('Before Validate')
if (await client.validateUrl(startUrl)) {
console.log('After Validate')
return { error: '', warning: ssoUrlFormatMessage }
} else {
return { error: urlInvalidFormatMessage, warning: '' }
}
}

// Ensure that URL does not exist yet
if (existingStartUrls.some((url) => url === startUrl)) {
return {
error: 'A connection for this start URL already exists. Sign out before creating a new one.',
warning: '',
}
}

// URL is valid
return { error: '', warning: '' }
}
},
handleRegionInput(event: any) {
void client.storeMetricMetadata({
Expand Down Expand Up @@ -743,6 +775,10 @@ body.vscode-high-contrast:not(body.vscode-high-contrast-light) .regionSelect {
color: #ff0000;
font-size: var(--font-size-sm);
}
.start-url-warning {
color: #dfe216;
font-size: var(--font-size-sm);
}
#logo {
fill: var(--vscode-button-foreground);
}
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/shared/utilities/uriUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,15 @@ export function toUri(path: string | vscode.Uri): vscode.Uri {
}
return vscode.Uri.file(path)
}

export function isValidUrl(string: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

is vscode.Uri.parse useful here? docstring may want to mention why not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would add additional LOC since we'd have to check every part manually. Also there may be nuances with a valid URL that we could miss

try {
const url = new URL(string)
// handle case where, eg: 'https://test', would be considered valid. At a minimum
// we'll require a top-level domain, eg: 'https://test.com'.
const hostParts = url.hostname.split('.').filter((part) => !!part)
return hostParts.length > 1
} catch (err) {
return false
}
}
14 changes: 13 additions & 1 deletion packages/core/src/test/shared/utilities/uriUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import assert from 'assert'
import { fromQueryToParameters } from '../../../shared/utilities/uriUtils'
import { fromQueryToParameters, isValidUrl } from '../../../shared/utilities/uriUtils'

describe('uriUtils', function () {
it('returns empty when no query parameters are present', function () {
Expand Down Expand Up @@ -33,4 +33,16 @@ describe('uriUtils', function () {
])
)
})

it('isValidUrl()', function () {
assert.strictEqual(isValidUrl(''), false)
assert.strictEqual(isValidUrl('test.com'), false)
assert.strictEqual(isValidUrl('https://test'), false)
assert.strictEqual(isValidUrl('http://test.'), false)
assert.strictEqual(isValidUrl('https://test.com http://test2.com'), false)

assert.strictEqual(isValidUrl('https://test.com'), true)
assert.strictEqual(isValidUrl('http://test.com'), true)
assert.strictEqual(isValidUrl('http://test.com/path'), true)
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "Auth: Valid StartURL not accepted at login"
}
Loading