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.

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

fix: add setting variable to determine request scheme #4438

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

zawan-ila
Copy link
Contributor

@zawan-ila zawan-ila commented Sep 9, 2024

PROD 4118

When we have paginated list endpoints, the "next" page URL is mistakenly HTTP. We suspect this is because the URL is constructed based on the current request scheme. edx-platform seems to workaround this issue by setting SECURE_PROXY_SSL_HEADER which overrides the current request scheme when DRF tries to fetch it to construct a next URL, so this PR is mimicking that behavior.

Enterprise also does the same, and that is where we mostly copy-pasted this PR from.

Internal Slack discussions for more context

https://twou.slack.com/archives/C04ACDVM6A1/p1720630664588599

https://twou.slack.com/archives/C049BVC75J7/p1720627508164049

Copy link
Contributor

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

recommend a comment about the motivation.

# a user can fool our server into thinking it was an https connection.
# See https://docs.djangoproject.com/en/5.1/ref/settings/#secure-proxy-ssl-header
# for other warnings.
SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
Copy link
Contributor

Choose a reason for hiding this comment

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

should we not set this in edx-internal for edX only as the server behind proxy might or might not be applicable for community?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I am just following what edx-platform and enterprise repos do. If platform does the same, we should be OK too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, fair point.

@zawan-ila zawan-ila merged commit 7e6e737 into openedx:master Sep 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants