v4 Signing Errors with exactly 7 day expiry

[JohnGale87]

Environment details

• OS: macOS 13.2.1
• Node.js version: 16.19.0
• npm version: 8.19.3
• @google-cloud/storage version: 6.9.4

Steps to reproduce

const FILE_NAME = '<your-file-name>';
const BUCKET_NAME = '<your-bucket-name>';
const file = new Storage().bucket(BUCKET_NAME).file(FILE_NAME);
const ACCESSIBLE_AT = new Date().setMilliseconds(999).valueOf();
const SEVEN_DAYS_IN_MS = 7 * 24 * 60 * 60 * 1000;
const [url] = await file.getSignedUrl({
  version: 'v4',
  action: 'read',
  accessibleAt: ACCESSIBLE_AT,
  expires: ACCESSIBLE_AT + SEVEN_DAYS_IN_MS,
  virtualHostedStyle: true,
});

The above code will throw Error: Max allowed expiration is seven days (604800 seconds)..

I've debugged it and found what I believe is a bug in src.signer.URLSigner.parseExpires that uses Math.round instead of Math.floor.

src.signer.URLSigner.parseAccessibleAt already uses Math.floor and does not seem to suffer with the same issue.

[JohnGale87]

I know there is a typo in the PR, I have fixed it locally but can't push right now because of this incident. Will push the fix asap.

generateKeyPairSyn c vs generateKeyPairSync

[JohnGale87]

GitHub incident resolved now, pushed my updated commits. Switched first commit to use conventional commit messages. Added a second commit to fix the aforementioned typo.

[ddelgrosso1]

Thanks for the report and fix @JohnGale87. I will take a look at this today and merge / release and/or leave feedback.

[ddelgrosso1]

So in looking at the above code I do not think this is a bug. expires is being set to ACCESSIBLE_AT + SEVEN_DAYS_IN_MS. The max value for expires is 604800.

Running this now would give:
new Date().setMilliseconds(999).valueOf() = 1680010307999

Which means expires = 1680010307999 + 604800.

Please correct me if I am misunderstanding something.

[JohnGale87]

So in looking at the above code I do not think this is a bug.

There certainly is scope to interperate it this way, however, I don't agree, let me try and explain in a more verbose way and hopefully you will see...?

expires is being set to ACCESSIBLE_AT + SEVEN_DAYS_IN_MS. The max value for expires is 604800.

No argument here, 100% agree

Running this now would give: new Date().setMilliseconds(999).valueOf() = 1680010307999
Which means expires = 1680010307999 + 604800.

Not exactly, you've miscaclualted between seconds and miliseconds and that is the crux of the issue.

it should be: 1680010307999 + 604800000.

The nodejs-storage library is internally using miliseconds but the external API is expecting seconds. In the conversion between milliseconds to seconds there is a rounding error which results in an over by one error.

So if I pass in:
accessibleAt = 1680010307999
expires: 1680010307999 + 604800000 = 1680615107999

The maxAge (in milliseconds) here is exactly 604800000 but due to rounding in parseExpires it is calculated to be 604800001

simplified version of existing parseExpires

parseExpires(
  expires: string | number | Date,
): number {
  const expiresInMSeconds = new Date(expires).valueOf();
  return Math.round(expiresInMSeconds / 1000);
}

Using the above parseExpires function and the above milisecond values we see the return value (in seconds now) is 1680615108 which makes the maxAge 604801 when the return value should be 1680615107

The 2 functions parseExpires and parseAccessibleAt both accept milisecond values but use different conversion methods to change them into seconds, one used Math.round and the other uses Math.floor when the library then compares the output of these two functions it finds that even though the inputs were valid their outputs are now invalid.

My PR proposes to use Math.floor for both parseExpires and parseAccessibleAt as that seems the most correct to me (considering the API doesn't care about milliseconds) however if you would prefer to change them to both use Math.round then that will fix the bug too. We just need to ensure they both use the same conversion logic whatever logic that is doesn't matter too much.

---

nodejs-storage Documentation says that the acceptable values for expiry are all passed into the Date constructor.

MDN Documentation for the Date constructor indicating that it accepts miliseconds

MDN Documentation for the valueOf method indicating that it returns miliseconds

[JohnGale87]

@ddelgrosso1 forgot to tag you in my reply, not sure if that is nessecary or not.

[JohnGale87]

I think this JSFiddle demonstrates the issue:

https://jsfiddle.net/4ugcsde7/2/

const sevenDays = 7 * 24 * 60 * 60 * 1000;
const accessibleAt = 1680010307999;
const expires = accessibleAt + sevenDays;

console.log('starting values', sevenDays, accessibleAt, expires);
console.log(
  'accessibleAt',
  Math.round(new Date(accessibleAt).valueOf() / 1000),
  Math.floor(new Date(accessibleAt).valueOf() / 1000)
);
console.log(
  'expires',
  Math.round(new Date(expires).valueOf() / 1000),
  Math.floor(new Date(expires).valueOf() / 1000)
);
console.log(
  'comparisons',
  Math.round(new Date(expires).valueOf() / 1000) -
    Math.round(new Date(accessibleAt).valueOf() / 1000),
  Math.floor(new Date(expires).valueOf() / 1000) -
    Math.floor(new Date(accessibleAt).valueOf() / 1000),
  Math.round(new Date(expires).valueOf() / 1000) -
    Math.floor(new Date(accessibleAt).valueOf() / 1000),
  Math.floor(new Date(expires).valueOf() / 1000) -
    Math.round(new Date(accessibleAt).valueOf() / 1000)
);

[ddelgrosso1]

@JohnGale87 thank you for the explanation! This is why I shouldn't do math first thing in the morning 😆 I agree that both of the utility functions should be utilizing the same conversion function and Math.floor makes logical sense to me. I am just going to run a few more tests but I think this looks good and will get it merged / released ASAP.

[ddelgrosso1]

This will be released with v6.9.5.