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(app-shell): sanitize correlation id parts #1057

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

pa3
Copy link
Contributor

@pa3 pa3 commented Sep 16, 2019

X-Correlation-ID header of requests sent to backend have to be -- quoting CT documentation -- "strings that may only contain alphanumeric characters, underscores and hyphens and have a length of 8 to 256 characters".

X-Correlation-ID is being built using user-provided data (namely, projectKey). In case this data does not satisfies above-mentioned constraints, request fails with an error, describing malformed correlation id. This error, in turn is being reported to Sentry, which spoils FLD job.

In order to avoid reporting this errors, this PR introduces correlation id sanitization. Original suggestion was to URL-encode provided projectKey, but that wont' work because, basically, % character would not pass X-Correlation-ID validation.

Instead, we just gonna drop malformed parts.

Paying for review

  1. [experiment] Try Roboto font-family #1056 (review)
  2. fix: use correct font family ui-kit#1061 (review)

@pa3 pa3 requested review from emmenko and montezume September 16, 2019 09:24
@pa3 pa3 added 🐛 Type: Bug Something isn't working 🙏 Status: Dev Review Waiting for technical reviews labels Sep 16, 2019
@@ -1,8 +1,13 @@
import uuid from 'uuid/v4';
import selectProjectKeyFromUrl from '../select-project-key-from-url';

const VALID_ID_PART_FORMAT = /^[\w-/]+$/;
Copy link
Contributor

@montezume montezume Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind adding a comment explaining what this does for those of us stupid enough not to be regex fluent 😅 . Or at least explain which elements would make it "malformed"

Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Starting to wonder if, as a follow up, and with the new middleware in our API in place we can remove the id altogether from here. I will look into that on the side some time (hopefully).

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@emmenko emmenko added the 🚀 Status: ship it Triggers an merge if rules match via bot label Sep 16, 2019
@kodiakhq kodiakhq bot merged commit e4b92cb into master Sep 16, 2019
@kodiakhq kodiakhq bot deleted the pa3-sanitize-correlation-id branch September 16, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 Status: Dev Review Waiting for technical reviews 🚀 Status: ship it Triggers an merge if rules match via bot 🐛 Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants