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

feat: enhance security against header spoofing #12213

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jahirvidrio
Copy link

This pull request introduces a significant enhancement to the security model of the output: 'server' or 'hybrid' mode in NodeApp. The primary goal is to provide developers with the option to control the trustworthiness of headers from downstream proxies.

Changes

Add trustDownstreamProxy option to enhance security against header spoofing

This change introduces a new optional parameter called trustDownstreamProxy to the function NodeApp.createRequest. This parameter allows developers to control whether the X-Forwarded-XXXX headers should be considered. By default, it is set to true, maintaining backward compatibility. When set to false, the headers are ignored, enhancing security by reducing the risk of header spoofing. This change improves the configuration options available to developers while ensuring that existing functionality remains unchanged.

Testing

There was no need to include tests for this change because the new trustDownstreamProxy option is optional and defaults to true, ensuring that the existing behavior of the function remains unchanged. This change focuses on enhancing configuration and security without altering the current logic of the code.

Docs

N/A: This change primarily impacts developers and maintainers of the project. The new trustDownstreamProxy option has been documented directly in the code using JSDoc comments. While there isn't separate user-facing documentation, the inline comments provide clear guidance on how to use this option and its purpose within the existing codebase.

Jahir Vidrio added 3 commits October 13, 2024 12:41
…t and improve documentation

* Moved inner function `functionName` outside its parent function for better code organization and readability.
* Improved JSDoc annotations for `functionName`, detailing parameters, return type, and usage.
…better highlighting

* Changed `//` comments to `/** */` for comments that include `@example` annotations.
* Ensured proper JSDoc format so that code editors correctly highlight the examples.
… security against header spoofing

* Introduced a new `trustDownstreamProxy` option in the `options` object to control whether `X-Forwarded-XXXX` headers should be considered.
* This boolean option defaults to `true` to maintain backward compatibility while enhancing security.
* When set to `false`, the `X-Forwarded` headers are ignored, reducing the risk of header spoofing.
Copy link

changeset-bot bot commented Oct 13, 2024

🦋 Changeset detected

Latest commit: e1c664d

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 13, 2024
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Oct 13, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@jahirvidrio
Copy link
Author

If this pull request is approved, I will take the initiative to create a corresponding pull request for the @astrojs/node package. The goal is to introduce a new environment variable, TRUST_DOWNSTREAM_PROXY, which will allow developers to specify the trustDownstreamProxy option in their calls to NodeApp.createRequest. This enhancement aims to further improve security and configurability within the framework.

@ematipico
Copy link
Member

If this pull request is approved, I will take the initiative to create a corresponding pull request for the @astrojs/node package. The goal is to introduce a new environment variable, TRUST_DOWNSTREAM_PROXY, which will allow developers to specify the trustDownstreamProxy option in their calls to NodeApp.createRequest. This enhancement aims to further improve security and configurability within the framework.

Does this mean that when TRUST_DOWNSTREAM_PROXY=false, the adapter won't read those headers?

Also, there is need of tests. We need to test cases where trustDownstreamProxy is false. Can you please provide them?

@jahirvidrio
Copy link
Author

Does this mean that when TRUST_DOWNSTREAM_PROXY=false, the adapter won't read those headers?

That’s right, all request values must be obtained from the socket and the IncomingMessage object.


We need to test cases where trustDownstreamProxy is false. Can you please provide them?

Of course, I'll try to create a test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants