This repository has been archived by the owner on Mar 10, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 299
feat: add support for custom headers to send-request #741
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
4ebb2e0
add support for custom headers to send-request
OR13 d8301a5
add custom headers test
OR13 7757d1a
add custom headers test
OR13 9311394
add custom header documentation
OR13 ee4368b
Merge branch 'master' into feat/custom-headers
cf96cee
fix: clone config.headers to avoid prev headers in next req
alanshaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,7 +106,7 @@ function requestAPI (config, options, callback) { | |
delete options.qs.followSymlinks | ||
|
||
const method = 'POST' | ||
const headers = {} | ||
const headers = config.headers || {} | ||
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. We need an |
||
|
||
if (isNode) { | ||
// Browsers do not allow you to modify the user agent | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/* eslint-env mocha */ | ||
'use strict' | ||
|
||
const isNode = require('detect-node') | ||
const chai = require('chai') | ||
const dirtyChai = require('dirty-chai') | ||
const expect = chai.expect | ||
chai.use(dirtyChai) | ||
|
||
const IPFSApi = require('../src') | ||
const f = require('./utils/factory') | ||
|
||
describe('custom headers', function () { | ||
// do not test in browser | ||
if (!isNode) { return } | ||
this.timeout(50 * 1000) // slow CI | ||
let ipfs | ||
let ipfsd | ||
// initialize ipfs with custom headers | ||
before(done => { | ||
f.spawn({ initOptions: { bits: 1024 } }, (err, _ipfsd) => { | ||
expect(err).to.not.exist() | ||
ipfsd = _ipfsd | ||
ipfs = IPFSApi({ | ||
host: 'localhost', | ||
port: 6001, | ||
protocol: 'http', | ||
headers: { | ||
authorization: 'Bearer ' + 'YOLO' | ||
} | ||
}) | ||
done() | ||
}) | ||
}) | ||
|
||
it('are supported', done => { | ||
// spin up a test http server to inspect the requests made by the library | ||
const server = require('http').createServer((req, res) => { | ||
req.on('data', () => {}) | ||
req.on('end', () => { | ||
res.writeHead(200) | ||
res.end() | ||
// ensure custom headers are present | ||
expect(req.headers.authorization).to.equal('Bearer ' + 'YOLO') | ||
server.close() | ||
done() | ||
}) | ||
}) | ||
|
||
server.listen(6001, () => { | ||
ipfs.id((err, res) => { | ||
if (err) { | ||
throw new Error('Unexpected error.') | ||
} | ||
// this call is used to test that headers are being sent. | ||
}) | ||
}) | ||
}) | ||
|
||
after(done => ipfsd.stop(done)) | ||
}) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not familiar with the http server lib we're using here -- does it transparently convert header keys to camel case?
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.
@lgierth I've often wondered about casing of http headers, especially across languages:
https://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive
Here are come examples of headers being set prior to this change.
https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L113
Its worth noting that user agent header will be overwritten by this code even if its provided in the config.
The headers object is passed directly to the request library:
https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L162
https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/request.js
Here is an interesting blog post on header casing and some challenges it can cause:
https://medium.com/walmartlabs/hacking-node-core-http-module-f2a10ea3028e
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.
long story short, i don't think header casing should be altered by code in this library.
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.
This is actually a pretty hard and annoying problem, particularly in Node.js.
The basic rule of thumb is that you need to preserve casing on user inputs. This is very problematic if you need to check and manipulate headers elsewhere in code.
I wrote a library to deal with this in
request
, so it's pretty widely used. https://github.com/request/caseless, basically just wraps the headers object and provides an API for you to manipulate it in a caseless way while still maintaining the case of the user inputs.