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: AWS credential signing http request - convert form to body #14060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zordrak
Copy link

@Zordrak Zordrak commented Mar 19, 2025

Summary

The code that signs a request using AWS credentials does not correctly handle objects passed from HTTP Request.

There is an in-progress refactor, as already worked around in #5751 / #8563 regarding the HTTP Request Options types in use. HTTP Request uses IRequestOptions, but AWS Credentials uses IHttpOptions, which is the target for the refactor, and compatible with an AWS Request object, which is what the signing code is expecting.

The problem is that HTTP Request converts or passes form data as a form property of the object. This is safely ignored by the type management and object processing, but it renders the content to sign incomplete. AWS SigV4 expects and requires the body to be part of the signing request (as well as the content-type header), and is ignorant of the form property.

Unless the form data is converted (back) to body content for the signing request, the result will be an incorrect signature that AWS will reject.

This bug can be simply reproduced:

  • Use an HTTP Request node
  • Set and associate valid AWS credentials
  • Set properties:
    • Method: POST
    • URL: https://iam.amazonaws.com
    • Authentication: Predefined credential type
    • Credential Type: AWS
    • AWS: <select valid credential>
    • Send Query: false
    • Send Headers: false
    • Send Body: true
    • Body Content Type: Form Urlencoded
    • Specify Body: Using single field
    • Body: Action=ListUsers&Version=2010-05-08

The resulting error will be:

403

{
  "Error": {
    "Code": "SignatureDoesNotMatch",
    "Message": "The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.",
    "Type": "Sender"
  },
  "RequestId": "<request id>"
}

This PR adopts the same workaround approach as used previously to take what it is given, and modify it accordingly so that the signing is successful. form content is converted to body content, and to ensure it is present as required (and with the right normalised lower casing), if the content-type header is not present it is set to application/x-www-form-urlencoded.

Related Linear tickets, Github issues, and Community forum posts

Relates to #8563
Relates to #5751
Relates (kinda) to #14037

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created. -- NOTE: Bugfix, no docfix to do.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

Sorry, something went wrong.

@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request in linear Issue or PR has been created in Linear for internal review labels Mar 19, 2025
@Joffcom
Copy link
Member

Joffcom commented Mar 19, 2025

Hey @Zordrak,

Thanks for the PR, We have created "GHC-1280" as the internal reference to get this reviewed.

One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team.

@dana-gill dana-gill self-requested a review March 20, 2025 09:31
Comment on lines +479 to +480
// ! Workaround as we still use the IRequestOptions interface which uses uri instead of url
// ! To change when we replace the interface with IHttpRequestOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ! Workaround as we still use the IRequestOptions interface which uses uri instead of url
// ! To change when we replace the interface with IHttpRequestOptions

@@ -139,5 +139,44 @@ describe('Aws Credential', () => {
expect(result.method).toBe('POST');
expect(result.url).toBe('https://iam.amazonaws.com/');
});

it('should handle an IRequestOptions object with form instead of body', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a test for when body exists as well? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member in linear Issue or PR has been created in Linear for internal review node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants