Shields: Redirector: [ERR_INVALID_CHAR]: Invalid character in header content ["Location"]

[sentry-io[bot]]

Sentry Issue: SHIELDS-3Y

TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["Location"]
  File "/home/m/shields/core/base-service/redirector.js", line 120, in camp.route
    ask.res.setHeader('Location', redirectUrl)
...
(5 additional frame(s) were not displayed)

[platan]

@calebcartwright Are you working on this?

[calebcartwright]

@platan - no I'm not. Seems like it could basically be any of the redirectors though, presumably one that's missing an encoding of one or more params

[paulmelnikow]

We could add some validation to Redirector that checks for valid URL characters and includes the name of the service in the error message.

Though it wouldn't hurt to take a pass through and look for unencoded parameters!

[platan]

ask.res.setHeader('Location', redirectUrl) throws an exception when redirect URL contains an invalid character. ask is a class from Scout Camp, and res is a Node.js class https://github.com/espadrine/sc/blob/master/doc/Readme.md#ask-and-augmented-request.
Valid characters for a header value are defined in https://github.com/nodejs/node/blob/v9.6.0/lib/_http_common.js#L245 (in v.9.4.0 definition is more complicated, but contains identical characters https://github.com/nodejs/node/blob/v9.4.0/lib/_http_common.js#L256).
Problem occurs when you put an encoded illegal character in a fragment of a request URI which is transformed to a path.
An example. https://github.com/badges/shields/blob/master/services/azure-devops/vso-redirect.service.js#L23-L28 has a definition:

    route: {
      base: 'vso/release',
      pattern: ':organization/:projectId/:definitionId/:environmentId',
    },
    transformPath: ({ organization, projectId, definitionId, environmentId }) =>
      `/azure-devops/release/${organization}/${projectId}/${definitionId}/${environmentId}`,

Putting %0a (encoded \n) in projectId will cause a problem:
http://localhost:8080/vso/release/to%0atodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/1/1.svg

But it's OK to put %0a a part of a request URI which is transformed to query param:
https://img.shields.io/codecov/c/token/abc123def%0a456/gh/codecov/private-example.svg

We have 25 redirectors defined in 19 files. I think most of them have this problem. More examples:
curl http://localhost:8080/vso/release/to%0atodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/1/1.svg -v
curl http://localhost:8080/codecov/c/token/abc123def456/gh/code%0acov/private-example.svg -v
curl http://localhost:8080/discourse/https/meta.discourse.org/stat%0aus.svg -v
curl http://localhost:8080/github/issues/detail/s/bad%0ages/shields/979.svg -v

If we want to handle such request properly we should encode every element of the path usingencodeURIComponent:

      `/azure-devops/release/${encodeURIComponent(organization)}/${encodeURIComponent(projectId)}/${encodeURIComponent(definitionId)}/${encodeURIComponent(environmentId)}`,

Alternatively we can leave it as it is and add a check for Location value:
https://github.com/badges/shields/compare/issue-4013-invalid-char-in-redirect?expand=1
Or we can mix both approaches.

What do you think?