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: Include trailing slash when passing empty query parameters. #1476

Merged
merged 1 commit into from
Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions lib/StripeResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,42 @@ StripeResource.prototype = {
validateRequest: null,

createFullPath(commandPath, urlData) {
return this._joinUrlParts([
this.basePath(urlData),
this.path(urlData),
typeof commandPath == 'function' ? commandPath(urlData) : commandPath,
]);
const urlParts = [this.basePath(urlData), this.path(urlData)];

if (typeof commandPath === 'function') {
const computedCommandPath = commandPath(urlData);
// If we have no actual command path, we just omit it to avoid adding a
// trailing slash. This is important for top-level listing requests, which
// do not have a command path.
if (computedCommandPath) {
urlParts.push(computedCommandPath);
}
} else {
urlParts.push(commandPath);
}

return this._joinUrlParts(urlParts);
},

// Creates a relative resource path with symbols left in (unlike
// createFullPath which takes some data to replace them with). For example it
// might produce: /invoices/{id}
createResourcePathWithSymbols(pathWithSymbols) {
return `/${this._joinUrlParts([this.resourcePath, pathWithSymbols || ''])}`;
// If there is no path beyond the resource path, we want to produce just
// /<resource path> rather than /<resource path>/.
if (pathWithSymbols) {
return `/${this._joinUrlParts([this.resourcePath, pathWithSymbols])}`;
} else {
return `/${this.resourcePath}`;
}
},

_joinUrlParts(parts) {
// Replace any accidentally doubled up slashes. This previously used
// path.join, which would do this as well. Unfortunately we need to do this
// as the functions for creating paths are technically part of the public
// interface and so we need to preserve backwards compatibility.
const path = parts.join('/').replace(/\/{2,}/g, '/');

// If the path ends with a /, we preserve the behavior of path.join and
// strip off the trailing / (eg. /v1/customers/ -> /v1/customers).
return path.endsWith('/') ? path.slice(0, -1) : path;
return parts.join('/').replace(/\/{2,}/g, '/');
},

// DEPRECATED: Here for backcompat in case users relied on this.
Expand Down
1 change: 1 addition & 0 deletions lib/makeRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ function getRequestOpts(self, requestArgs, spec, overrideData) {
const requestPath = isUsingFullPath
? commandPath(urlData)
: self.createFullPath(commandPath, urlData);

const headers = Object.assign(options.headers, spec.headers);

if (spec.validator) {
Expand Down
79 changes: 76 additions & 3 deletions test/StripeResource.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,19 @@ const {

describe('StripeResource', () => {
describe('createResourcePathWithSymbols', () => {
it('Generates a path', () => {
it('Generates a path when there is a symbol', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('{id}');
expect(path).to.equal('/invoices/{id}');
});

it('Generates a path when there is nothing beyond the resource path', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('');
// This explicitly shouldn't have a trailing slash.
expect(path).to.equal('/invoices');
});

it('Handles accidental double slashes', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('/{id}');
Expand All @@ -36,9 +43,9 @@ describe('StripeResource', () => {
});

describe('_joinUrlParts', () => {
it('handles trailing empty values correctly', () => {
it('includes trailing empty values', () => {
const path = stripe.invoices._joinUrlParts(['a', '']);
expect(path).to.equal('a');
expect(path).to.equal('a/');
});

it('joins parts', () => {
Expand Down Expand Up @@ -132,6 +139,72 @@ describe('StripeResource', () => {
});
});

it('handles .. as a query param', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
.get('/v1/customers/..', '')
.reply(200, '{}');

realStripe.customers.retrieve('..', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/customers/', '')
.reply(200, '{}');

realStripe.customers.retrieve('', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param for namespaced resources', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/checkout/sessions/', '')
.reply(200, '{}');

realStripe.checkout.sessions.retrieve('', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param for nested resources', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/customers/cus_123/balance_transactions/', '')
.reply(200, '{}');

realStripe.customers.retrieveBalanceTransaction(
'cus_123',
'',
(err, response) => {
done(err);
scope.done();
}
);
});

it('does not include trailing slash for endpoints without query parameters', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note that no trailing slash is present.
.get('/v1/customers', '')
.reply(200, '{}');

realStripe.customers.list((err, response) => {
done(err);
scope.done();
});
});

it('works correctly with undefined optional arguments', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
.get('/v1/accounts/acct_123')
Expand Down