Mutating inputs can result in broken requests

[ryanblock]

Description

aws4 may mutates its inputs when signing requests instead of creating and returning new data. This may cause issues if, for example, a single headers object is used when signing multiple requests.

Reproduction steps

In this example, two requests are signed, yet the content-length from the first request is carried over to the second, resulting in a broken HTTP request:

import aws4 from 'aws4'

// All our requests will use the same headers
const headers = { 'content-type': 'application/json' }

const creds = {
  accessKeyId: 'abc',
  secretAccessKey: '123',
}
const boilerplate = {
  region: 'us-west-1',
  service: 'lambda',
}

// First request
const signing1 = {
  ...boilerplate,
  headers,
  body: '{"helloooooooooooo": true}',
}
const signed1 = aws4.sign(signing1, creds)
console.log('signing result 1', signed1)
console.log('content length:', headers['Content-Length'])
console.log('body size:     ', signing1.body.length)
console.log(`these two numbers should be the same, are they?`, headers['Content-Length'] === signing1.body.length)
console.log('\n')

// Second request
const signing2 = {
  ...boilerplate,
  headers,
  body: '{"hi": true}',
}
const signed2 = aws4.sign(signing2, creds)
console.log('signing result 2', signed2)
console.log('content length:', headers['Content-Length'])
console.log('body size:     ', signing2.body.length)
console.log(`these two numbers should be the same, are they?`, headers['Content-Length'] === signing2.body.length)

To me, ideally, my inputs (e.g. headers) should not be mutated when passed to aws4 for signing.

I have not audited the entire codebase for other potential mutations of inputs beyond headers (link), but hopefully this issue is generic enough to accurately describe a variety of errata we've been seeing in aws-lite.

Thanks! <3

[mhart]

Yes, it's part of the (current) API that it mutates its inputs instead of returning new ones. This was a deliberate decision so that aws4.sign(opts) operates on opts. The fact that it happens to also return opts was a convenience. Similar to Array.sort. To avoid this, the library would need to deep copy its inputs, which would also have a performance impact – instead it leaves that up to the user if they wish to reuse inputs (so I'd suggest aws-lite do this if you want to avoid things like this)

That said – the fact that aws4.sign isn't a void function is definitely confusing – and just like Array.sort, I get that it can lead to issues if the user/consumer is unaware of this.

It would be a breaking change, so would have to be in scope for a v2

[mhart]

Thinking about this some more. I wonder if it's ok to have both. I think the main concern is just modifying the nested headers object, right? So I think it wouldn't be too bad for aws4 to copy that. So opts gets modified in place, and returned, as it does today. The external API wouldn't change. Just that opts.headers would be a different object on the way than what it was on the way in.

I think that might be ok to update on a minor.

[mhart]

(I note your fix for aws-lite has a JSON.parse/JSON.stringify combo – I wouldn't do that for perf reasons tbh, especially for large bodies – plus you never know if ppl are passing in weird inputs that don't JSON well)

[mhart]

Ok, published as 1.13.0 🙌 – now, go remove that awful JSON.parse(JSON.stringify()) monstrosity 😄

[ryanblock]

@mhart ha, yeah, was 5 hours into tracing this bug on a holiday, and brain just ready to not brain anymore. 😅

Thank you!

[mhart]

Goddammit. #165 https://xkcd.com/1172/

[ryanblock]

@mhart omg that xkcd, far too true. In all seriousness: I'll def advocate in favor of your 1.13 release as-is – it's the correct approach (in my opinion) and sometimes bug fixes do introduce breaking changes. That said, if you opt to revert it and re-publish as 2.0 (or whatever), I'm totally fine with that, it's all good. Whatever is most favorable to your mental and emotional wellbeing! 💕