-
Notifications
You must be signed in to change notification settings - Fork 370
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: v4 Signing Errors with exactly 7 day expiry #2170
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -29,7 +29,7 @@ import { | |||
SignerExceptionMessages, | ||||
} from '../src/signer'; | ||||
import {encodeURI, formatAsUTCISO, qsStringify} from '../src/util'; | ||||
import {ExceptionMessages} from '../src/storage'; | ||||
import {Storage, ExceptionMessages} from '../src/storage'; | ||||
import {OutgoingHttpHeaders} from 'http'; | ||||
|
||||
interface SignedUrlArgs { | ||||
|
@@ -56,7 +56,7 @@ describe('signer', () => { | |||
let bucket: BucketI; | ||||
let file: FileI; | ||||
|
||||
const NOW = new Date('2019-03-18T00:00:00Z'); | ||||
const NOW = new Date('2019-03-18T00:00:00.999Z'); | ||||
let fakeTimers: sinon.SinonFakeTimers; | ||||
|
||||
beforeEach(() => (fakeTimers = sinon.useFakeTimers(NOW))); | ||||
|
@@ -554,7 +554,7 @@ describe('signer', () => { | |||
signer = new URLSigner(authClient, bucket, file); | ||||
CONFIG = { | ||||
method: 'GET', | ||||
expiration: Math.floor((NOW.valueOf() + 2000) / 1000), | ||||
expiration: (NOW.valueOf() + 2000) / 1000, | ||||
bucket: bucket.name, | ||||
}; | ||||
}); | ||||
|
@@ -573,6 +573,52 @@ describe('signer', () => { | |||
); | ||||
}); | ||||
|
||||
it('should not throw with expiration of exactly 7 days', async () => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if this should be a unittest or a system test so I added it as both. This unittest tests the signer by using the file object so isn't really a "unit" test however this was the closest thing to my real world problem that I could think of testing. N.B. Even with the private key generation, mocha does not flag this unittest as a long running run so I figured it was ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much appreciated adding both. We have the timeout set as 10s for these tests. |
||||
const file = new Storage({ | ||||
projectId: 'xxxx', | ||||
credentials: { | ||||
type: 'service_account', | ||||
private_key: crypto.generateKeyPairSync('rsa', { | ||||
modulusLength: 512, | ||||
publicKeyEncoding: { | ||||
type: 'spki', | ||||
format: 'pem', | ||||
}, | ||||
privateKeyEncoding: { | ||||
type: 'pkcs8', | ||||
format: 'pem', | ||||
}, | ||||
}).privateKey, | ||||
client_email: 'xxxx', | ||||
client_id: 'xxx', | ||||
}, | ||||
}) | ||||
.bucket(BUCKET_NAME) | ||||
.file(FILE_NAME); | ||||
const ACCESSIBLE_AT = NOW.valueOf(); | ||||
const SEVEN_DAYS_IN_SECONDS = 7 * 24 * 60 * 60; | ||||
const SEVEN_DAYS_IN_MS = SEVEN_DAYS_IN_SECONDS * 1000; | ||||
await assert.doesNotReject( | ||||
async () => { | ||||
await file.getSignedUrl({ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be accomplished without the need of pulling in Line 208 in 2c967f6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes agreed. Please see latest commit. |
||||
version: 'v4', | ||||
action: 'read', | ||||
accessibleAt: ACCESSIBLE_AT, | ||||
expires: ACCESSIBLE_AT + SEVEN_DAYS_IN_MS, | ||||
virtualHostedStyle: true, | ||||
}); | ||||
}, | ||||
err => { | ||||
assert(err instanceof Error); | ||||
assert.strictEqual( | ||||
err.message, | ||||
`Max allowed expiration is seven days (${SEVEN_DAYS_IN_SECONDS} seconds).` | ||||
); | ||||
return true; | ||||
} | ||||
); | ||||
}); | ||||
|
||||
describe('headers', () => { | ||||
it('should add path-styled host header', async () => { | ||||
const getCanonicalHeaders = sandbox | ||||
|
@@ -988,7 +1034,7 @@ describe('signer', () => { | |||
|
||||
it('returns expiration date in seconds', () => { | ||||
const expires = signer.parseExpires(NOW); | ||||
assert.strictEqual(expires, Math.round(NOW.valueOf() / 1000)); | ||||
assert.strictEqual(expires, Math.floor(NOW.valueOf() / 1000)); | ||||
}); | ||||
}); | ||||
}); | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only values >= 500 trigger the error, if the ms date part is in the range 0 - 499 then
Math.round
functions correctly.