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

EDSC-3930: Fixing issue on EDSC #1698

Merged
merged 5 commits into from
Dec 15, 2023
Merged

EDSC-3930: Fixing issue on EDSC #1698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b341900
EDSC-3930: Make redirects relative only
eudoroolivares2016 Dec 4, 2023
1af9f0d
EDSC-3930: refactor and clean
eudoroolivares2016 Dec 4, 2023
272a7c3
EDSC-3930: Add spacing; redirect invalid urls to the not-found EDSC p…
eudoroolivares2016 Dec 5, 2023
26d56f1
EDSC-3930: Update test to redirect to not found page
eudoroolivares2016 Dec 5, 2023
15b636e
EDSC-3930: removing extraneous comment, adding explicit test
eudoroolivares2016 Dec 12, 2023
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
58 changes: 30 additions & 28 deletions static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { set } from 'tiny-cookie'
import { connect } from 'react-redux'
import { parse } from 'qs'

import { getEnvironmentConfig } from '../../../../../sharedUtils/config'
import { locationPropType } from '../../util/propTypes/location'
import history from '../../util/history'

Expand All @@ -28,6 +29,8 @@ export const AuthCallbackContainer = ({
location,
onAddEarthdataDownloadRedirect
}) => {
const { edscHost } = getEnvironmentConfig()

useEffect(() => {
const { search } = location

Expand All @@ -39,42 +42,41 @@ export const AuthCallbackContainer = ({
redirect = '/'
} = params

// Verify that the redirect params are real URLs
try {
let redirectUrl
if (eddRedirect) redirectUrl = new URL(eddRedirect)
if (redirect && redirect !== '/') redirectUrl = new URL(redirect)

if (
redirectUrl
&& redirectUrl.protocol !== 'http:'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like we lost where were checking for the allowed protocols. Is that intentional?

Copy link
Contributor Author

@eudoroolivares2016 eudoroolivares2016 Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yea that is intentional. The protocol check doesn't sufficiently cover the strictness we need to properly handle url redirects. Here we are now only allowing redirects to addresses relative to EDSC or earthdata-download

&& redirectUrl.protocol !== 'https:'
&& redirectUrl.protocol !== 'earthdata-download:'
) {
// The redirectUrl is not a valid protocol
console.log('The redirectUrl is not a valid protocol')
window.location.replace('/')
let eddRedirectUrl = eddRedirect

if (redirect.includes('earthdata-download')) {
eudoroolivares2016 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to do an exact match on the url just in case we use "earthdata-download" in a url sometime down the line. Seems safer to be a little more specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

One possibility is moving the test on line 53 with startsWith up here, which would mean only valid EDD redirects get processed by the following if block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rushgeo We could check if it starts with up there for redirect but, we still need to have a startsWith check in case the eddRedirectUrl has been initialized to the value of the eddRedirect. I used includes because that was previous logic.

eddRedirectUrl = redirect
}

// Handle EDD redirects
if (eddRedirectUrl) {
const validEddRedirect = eddRedirectUrl.startsWith('earthdata-download')
eudoroolivares2016 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to do an exact match on the url just in case we use "earthdata-download" in a url sometime down the line. Seems safer to be a little more specific.

Copy link
Contributor Author

@eudoroolivares2016 eudoroolivares2016 Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A little confused about what you mean for the exact matching. We wouldn't know what the eddRedirectUrl value would be only its prefix at runtime of this module?

Copy link
Contributor Author

@eudoroolivares2016 eudoroolivares2016 Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For instance one value I see in testing with downloading a collection where EDL auth was required is earthdata-download://authCallback?fileId=3&ee=prod&token=<my-token>
I'm not sure we know what the fileId is going to be or really that the token matches whatever token we have saved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea so the fileId coming from EDD is The authUrl will have a fileId parameter appended to it, which is the file ID of the file that triggered the authentication workflow


if (validEddRedirect) {
if (accessToken) eddRedirectUrl += `&token=${accessToken}`

// Add the redirect information to the store
onAddEarthdataDownloadRedirect({
redirect: eddRedirectUrl
})

// Redirect to the edd callback
history.push('/earthdata-download-callback')

return
}
} catch (error) {
window.location.replace('/')

window.location.replace('/not-found')

return
}

// If the redirect includes earthdata-download, redirect to the edd callback
if (eddRedirect || redirect.includes('earthdata-download')) {
let eddRedirectUrl = eddRedirect || redirect
if (accessToken) eddRedirectUrl += `&token=${accessToken}`

// Add the redirect information to the store
onAddEarthdataDownloadRedirect({
redirect: eddRedirectUrl
})
// Handle redirects
const invalidRedirectUrl = redirect !== '/' && !redirect.startsWith(edscHost)

// Redirect to the edd callback
history.push('/earthdata-download-callback')
if (invalidRedirectUrl) {
// Redirect to an error page or a safe location if the URL is not a relative path
eudoroolivares2016 marked this conversation as resolved.
Show resolved Hide resolved
window.location.replace('/not-found')

return
}
Expand Down
38 changes: 36 additions & 2 deletions static/src/js/containers/AuthCallbackContainer/__tests__/AuthCallbackContainer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,41 @@ describe('AuthCallbackContainer component', () => {
expect(setSpy).toBeCalledTimes(0)

expect(window.location.replace.mock.calls.length).toBe(1)
expect(window.location.replace.mock.calls[0]).toEqual(['/'])
expect(window.location.replace.mock.calls[0]).toEqual(['/not-found'])
})

test('does not follow the redirect if the redirect param is not relative to earthdata-search', () => {
const setSpy = jest.spyOn(tinyCookie, 'set')
delete window.location
window.location = { replace: jest.fn() }

setup({
location: {
search: '?redirect=https://evil.com'
}
})

expect(setSpy).toBeCalledTimes(0)

expect(window.location.replace.mock.calls.length).toBe(1)
expect(window.location.replace.mock.calls[0]).toEqual(['/not-found'])
})

test('does not follow the eddRedirect it is not a valid earthdata-download redirect', () => {
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 don't know that this test is really very helpful. its very similar to the test below it and running though the same code. I added it for explicitness when it comes to what solution this PR is trying to do but, I'll pull it out if people deem to totally redundant

const setSpy = jest.spyOn(tinyCookie, 'set')
delete window.location
window.location = { replace: jest.fn() }

setup({
location: {
search: '?eddRedirect=https://evil.com'
}
})

expect(setSpy).toBeCalledTimes(0)

expect(window.location.replace.mock.calls.length).toBe(1)
expect(window.location.replace.mock.calls[0]).toEqual(['/not-found'])
})

test('does not follow the redirect if the eddRedirect param is not valid', () => {
Expand All @@ -171,6 +205,6 @@ describe('AuthCallbackContainer component', () => {
expect(setSpy).toBeCalledTimes(0)

expect(window.location.replace.mock.calls.length).toBe(1)
expect(window.location.replace.mock.calls[0]).toEqual(['/'])
expect(window.location.replace.mock.calls[0]).toEqual(['/not-found'])
})
})
Loading