EDSC-3930: Fixing issue on EDSC #1698

eudoroolivares2016 · 2023-12-04T22:02:02Z

Overview

What is the feature?

Fix an issue on EDSC (Further info in the Teams file on this issue) this has been left purposefully vague

What is the Solution?

Altered parsing of redirect urls for strictness

Regression testing was done locally to ensure that we could still be redirected from EDD to URS and to the EULA page using the prod collection C1214470488-ASF with a profile that had not accepted any Eulas

We can check C2202497474-LPCLOUD collection to ensure that EDD redirects through URS are getting sent as expected

What areas of the application does this impact?

List impacted areas.

EDD redirects
EDL redirects

Reproduction steps

(Further info in the Teams file on this issue) this has been left purposefully vague

Attachments

N/A

Checklist

I have added automated tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
[n/a] I have made corresponding changes to the documentation
My changes generate no new warnings

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

trevorlang · 2023-12-05T15:05:45Z

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

+
+    // Handle EDD redirects
+    if (eddRedirectUrl) {
+      const validEddRedirect = eddRedirectUrl.startsWith('earthdata-download')


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.

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?

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

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

trevorlang · 2023-12-05T15:06:26Z

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

-        console.log('The redirectUrl is not a valid protocol')
-        window.location.replace('/')
+    let eddRedirectUrl = eddRedirect
+    if (redirect.includes('earthdata-download')) {


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.

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

@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.

trevorlang · 2023-12-05T15:11:41Z

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

+    if (invalidRedirectUrl) {
+      // Redirect to an error page or a safe location if the URL is not a relative path
+      // https://developer.mozilla.org/en-US/docs/Web/API/Location/replace assign prevents back-button use in history
+      window.location.replace('/')


It seems like we could send the user to the standard error page here. I think a "replace" makes more sense than a "assign" here.

trevorlang · 2023-12-05T15:27:06Z

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

-
-      if (
-        redirectUrl
-        && redirectUrl.protocol !== 'http:'


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

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

codecov · 2023-12-05T16:21:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bdb411f) 91.93% compared to head (15b636e) 91.94%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1698   +/-   ##
=======================================
  Coverage   91.93%   91.94%           
=======================================
  Files         725      725           
  Lines       19349    19347    -2     
  Branches     4562     4558    -4     
=======================================
  Hits        17789    17789           
+ Misses       1424     1422    -2     
  Partials      136      136

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rushgeo

Other than Trevor and I's comments on includes('earthdata-download'), it looks good to me.

rushgeo · 2023-12-11T21:27:37Z

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js

-        console.log('The redirectUrl is not a valid protocol')
-        window.location.replace('/')
+    let eddRedirectUrl = eddRedirect
+    if (redirect.includes('earthdata-download')) {


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

…age instead of home

eudoroolivares2016 · 2023-12-12T16:27:06Z

static/src/js/containers/AuthCallbackContainer/__tests__/AuthCallbackContainer.test.js

+    expect(window.location.replace.mock.calls[0]).toEqual(['/not-found'])
+  })
+
+  test('does not follow the eddRedirect it is not a valid earthdata-download redirect', () => {


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

eudoroolivares2016 commented Dec 4, 2023

View reviewed changes

static/src/js/containers/AuthCallbackContainer/AuthCallbackContainer.js Show resolved Hide resolved

trevorlang reviewed Dec 5, 2023

View reviewed changes

eudoroolivares2016 requested review from macrouch, Bccorb, bnp26, rushgeo, dpesall, daniel-zamora and trevorlang December 7, 2023 15:28

rushgeo reviewed Dec 11, 2023

View reviewed changes

eudoroolivares2016 added 5 commits December 12, 2023 11:25

EDSC-3930: Make redirects relative only

b341900

EDSC-3930: refactor and clean

1af9f0d

EDSC-3930: Add spacing; redirect invalid urls to the not-found EDSC p…

272a7c3

…age instead of home

EDSC-3930: Update test to redirect to not found page

26d56f1

EDSC-3930: removing extraneous comment, adding explicit test

15b636e

eudoroolivares2016 force-pushed the EDSC-3930 branch from a82b6ba to 15b636e Compare December 12, 2023 16:25

eudoroolivares2016 commented Dec 12, 2023

View reviewed changes

rushgeo approved these changes Dec 12, 2023

View reviewed changes

dpesall approved these changes Dec 13, 2023

View reviewed changes

eudoroolivares2016 merged commit 5cf217c into main Dec 15, 2023
9 checks passed

eudoroolivares2016 deleted the EDSC-3930 branch December 15, 2023 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EDSC-3930: Fixing issue on EDSC #1698

EDSC-3930: Fixing issue on EDSC #1698

eudoroolivares2016 commented Dec 4, 2023 •

edited

Loading

trevorlang Dec 5, 2023

eudoroolivares2016 Dec 5, 2023 •

edited

Loading

eudoroolivares2016 Dec 6, 2023 •

edited

Loading

eudoroolivares2016 Dec 6, 2023

trevorlang Dec 5, 2023

rushgeo Dec 11, 2023

eudoroolivares2016 Dec 12, 2023

trevorlang Dec 5, 2023

trevorlang Dec 5, 2023

eudoroolivares2016 Dec 5, 2023 •

edited

Loading

codecov bot commented Dec 5, 2023 •

edited

Loading

rushgeo left a comment

rushgeo Dec 11, 2023

eudoroolivares2016 Dec 12, 2023

EDSC-3930: Fixing issue on EDSC #1698

EDSC-3930: Fixing issue on EDSC #1698

Conversation

eudoroolivares2016 commented Dec 4, 2023 • edited Loading

Overview

What is the feature?

What is the Solution?

What areas of the application does this impact?

Reproduction steps

Attachments

Checklist

Choose a reason for hiding this comment

eudoroolivares2016 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

eudoroolivares2016 Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eudoroolivares2016 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

rushgeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eudoroolivares2016 commented Dec 4, 2023 •

edited

Loading

eudoroolivares2016 Dec 5, 2023 •

edited

Loading

eudoroolivares2016 Dec 6, 2023 •

edited

Loading

eudoroolivares2016 Dec 5, 2023 •

edited

Loading

codecov bot commented Dec 5, 2023 •

edited

Loading