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.

Sign up for GitHub

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

Jump to bottom

Redact More Variants Of Paths In Stack Traces #2229

Merged
merged 8 commits into from
Apr 11, 2018
Merged

Redact More Variants Of Paths In Stack Traces #2229

merged 8 commits into from
Apr 11, 2018

Conversation

gasi-signal
Copy link
Contributor

@gasi-signal gasi-signal commented Apr 6, 2018 •
edited
Loading

  • Redact stack traces with both forward and backslashes.
  • Redact paths with escaped forward slashes.
  • Redact URL-encoded paths.
  • Minor: Use is vs Lodash is* for type checking.
  • Minor: Rename Path to path, etc.
  • Add ESLint quotes rule to allow double quotes to avoid escaping single quotes.
  • Consistently use single quotes to denote identifiers in error messages, e.g. 'foo' is required.

@gasi-signal gasi-signal force-pushed the redact-more-stack-trace-paths branch from 7fc2be0 to e5dbb73 Compare April 6, 2018 18:41
@gasi-signal
Copy link
Contributor Author

@scottnonnenberg-signal This is ready for review.

scottnonnenberg-signal
scottnonnenberg-signal approved these changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@scottnonnenberg-signal scottnonnenberg-signal left a comment

Choose a reason for hiding this comment

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

Wow, good work putting all of these different escaping mechanisms in place!

js/modules/privacy.js Outdated
// _redactPath :: Path -> String -> String
exports._redactPath = (filePath) => {
if (!is.string(filePath)) {
throw new TypeError('"filePath" must be a string');
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be good to make that eslint change so we can move to "'filePath' must be a string"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming up in another PR.

@gasi-signal gasi-signal force-pushed the redact-more-stack-trace-paths branch from e5dbb73 to 432a6eb Compare April 11, 2018 19:37
@gasi-signal
Copy link
Contributor Author

@scottnonnenberg-signal Thanks for review! Ended up adding quotes ESLint rule and fixing up quoting of identifiers: https://github.com/signalapp/Signal-Desktop/pull/2229/files/432a6ebd7f549498a9ec71831654e5491689e709..24f4ad53bcf4734c3182dc1052144e5f40833bc5

@scottnonnenberg-signal
Copy link
Contributor

Looks good. Not sure if typescript would like this, but it would be nice to have some helpers where we pass in the type we want, and the variable, and it would take care of doing the throw if needed. Lots of boilerplate today.

@gasi-signal
Copy link
Contributor Author

Looks good. Not sure if typescript would like this, but it would be nice to have some helpers where we pass in the type we want, and the variable, and it would take care of doing the throw if needed. Lots of boilerplate today.

Good idea! I wrote a module in the past called Ensure that did that. But the beauty is that new TypeScript won’t even require this as runtime checks will be replaced by compile-time checks. For existing code we could do something like Ensure.isFunction('isActive', isActive); (not sure there is a way to create label from a value passed in without a compiler/pre-processor).

@gasi-signal gasi-signal merged commit 9b22e06 into development Apr 11, 2018
@gasi-signal gasi-signal deleted the redact-more-stack-trace-paths branch April 11, 2018 20:35
scottnonnenberg-signal added a commit that referenced this pull request Apr 13, 2018
@scottnonnenberg-signal
v1.7.1
c42c9cb 
Fixed: Conversation message preview would sometimes continue to show after message disappeared (1206b3c)

Improve URL Auto-Linking In Messages (#2240)

Redact More Variants Of Paths In Stack Traces (#2229)

Dev: Introduce React, TypeScript, TSLint and React-StyleGuidist (#2219 and #2232)
scottnonnenberg-signal added a commit that referenced this pull request Apr 16, 2018
@scottnonnenberg-signal
v1.8.0-beta.1
3dbc550 
Receive quoted replies (#2244)

iOS theme: one bubble for both attachment and message contents (#2244)

Improve URL Auto-Linking In Messages (#2240)

Redact More Variants Of Paths In Stack Traces (#2229)

Fixed: Conversation message preview would sometimes continue to show after message disappeared (1206b3c)

Dev: Introduce React, TypeScript, TSLint and React-StyleGuidist (#2219 and #2232)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottnonnenberg-signal scottnonnenberg-signal scottnonnenberg-signal approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gasi-signal @scottnonnenberg-signal