Skip to content

Use values from x-forwarded headers in getOrigin server side#1850

Merged
tdonohue merged 2 commits intoDSpace:mainfrom
atmire:use-x-forwarded-for-redirect
Sep 28, 2022
Merged

Use values from x-forwarded headers in getOrigin server side#1850
tdonohue merged 2 commits intoDSpace:mainfrom
atmire:use-x-forwarded-for-redirect

Conversation

@artlowel
Copy link
Copy Markdown
Member

@artlowel artlowel commented Sep 22, 2022

References

Description

This PR fixes #1721 by checking for the presence of the x-forwarded-proto and x-forwarded-host headers server side, and using them instead of the default host and protocol properties on the request if they are available.

To stay on par with #1725, I also added the same small extra fix to ensure a redirect ends up in the browser's history

Instructions for Reviewers

Run the app in prod mode. And test it with and without a proxy server in front that's configured to use the x-forwarded headers

Test that the citation_pdf_url on an item page always contains a URL that uses with the correct host and protocol, with and without using a proxy server .

To test the redirect change, download a file, and use the back button of your browser. You should end up back on the item page

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.

@artlowel artlowel added bug component: SEO Search Engine Optimization high priority 1 APPROVAL pull request only requires a single approval to merge labels Sep 22, 2022
@artlowel artlowel added this to the 7.4 milestone Sep 22, 2022
@artlowel artlowel self-assigned this Sep 22, 2022
@artlowel artlowel mentioned this pull request Sep 22, 2022
5 tasks
@tdonohue tdonohue requested a review from atarix83 September 22, 2022 14:07
tdonohue
tdonohue previously approved these changes Sep 23, 2022
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍Thanks @artlowel ! I tested this today and verified it works just as well as #1725, and I like this approach much better (as it aligns with standard HTTP headers).

Here's the testing process that I used:

  • Starting from main, I spun up the UI locally in production mode behind a proxy that uses HTTPS & X-Forwarded headers.
  • Accessed an Item page from the UI.
  • Then, turned off Javascript in my browser & reloaded the page.
  • Verified that citation_pdf_url wrongly uses http instead of https. (This was just to verify the bug still exists and it does)
  • Then, rebuilt/redeployed the UI using this PR & repeated these same steps
  • Verified that citation_pdf_url now uses https, even with JavaScript off.

So, this fixes the issue! Thanks again for figuring out a solution that uses X-Forwarded-* headers.

@abollini
Copy link
Copy Markdown
Member

I haven't had the chance to ask to @atarix83 for an opinion but I was just searching if there is anything "built-in" in angular to deal with proxy as it was a bit surprising for me that we need to write our own solution, I found this webpage that seems to indicate a built-in mechanism in expressjs (is it what we use right?)
https://expressjs.com/en/guide/behind-proxies.html

Anyway, I'm ok with the approach in this PR also if we should end to make our own implementation

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Sep 27, 2022

@artlowel and @abollini : I tried out @abollini 's idea, and it appears that we may be able to use the built-in approach in Express.

Here's what I tried:

  1. First, starting with the main branch, I modified server.ts adding this..
    export function app() {
      ...
      // Tell Express to trust X-FORWARDED-* headers from proxies
      // See https://expressjs.com/en/guide/behind-proxies.html
      server.set('trust proxy', true);
      ...
    }
    
  2. Then, I rebuilt & redeployed (yarn build:prod; yarn serve:ssr) and ran everything behind a proxy that uses X-FORWARDED-* headers
  3. I tested using the same mechanism as before. I turned off JS in my browser (to force SSR) and then checked the citation_pdf_url and IT WORKS. I'm seeing HTTPS instead of HTTP.

@artlowel : Could you see if this also works for you?

If it does, we can likely remove the custom code and either hardcode this setting to true, or add a new useProxies = true/false setting to config.*.yml (which would mirror the same useProxies setting we have on the backend in dspace.cfg.)

@artlowel artlowel force-pushed the use-x-forwarded-for-redirect branch from 556c132 to 941e71a Compare September 28, 2022 11:10
@artlowel
Copy link
Copy Markdown
Member Author

Yes that works

I updated the PR to use it instead

@artlowel artlowel requested a review from tdonohue September 28, 2022 12:37
@artlowel artlowel dismissed tdonohue’s stale review September 28, 2022 12:38

The code changed

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @artlowel

we have tested the fix works properly

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @artlowel ! Retested this today and it works perfectly. Code looks good to me too, and I agree with defaulting this useProxies setting to true (just like on the backend).

@tdonohue tdonohue merged commit 695ce3a into DSpace:main Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug component: SEO Search Engine Optimization high priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The "citation_pdf_url" meta tag download link doesn't use https

4 participants