Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Sideloader: no line breaks in base64 content #3038

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
Merged
merged 1 commit into from
Aug 31, 2021
Merged

Sideloader: no line breaks in base64 content #3038

merged 1 commit into from
Aug 31, 2021

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Aug 31, 2021 •
edited
Loading

Currently we are sending sideloaded resources as line-wrapped base64. The extra
newlines confuse the nREPL sideloader, turning the result into binary garbage.

I think in this case we should be both more conservative in what we send (this
patch) and more lenient in what we receive (nREPL should sanitize the input and
drop any characters that aren't valid base64).

Part of #3037

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@plexus
Sideloader: no line breaks in base64 content
45a4c6d 
Currently we are sending sideloaded resources as line-wrapped base64. The extra
newlines confuse the nREPL sideloader, turning the result into binary garbage.

I think in this case we should be both more conservative in what we send (this
patch) and more lenient in what we receive (nREPL should sanitize the input and
drop any characters that aren't valid base64).
@plexus
Copy link
Contributor Author

plexus commented Aug 31, 2021

The linter does not currently pass for me on master, but this PR does not introduce any new linting warnings. This PR does not add user visible functionality that needs to be mentioned in the manual.

@plexus
Copy link
Contributor Author

plexus commented Aug 31, 2021

Seems the linting does pass on CI. I installed eldev as per https://github.com/doublep/eldev#installation , and eldev lint gives me a bunch of warnings like

cider-doc.el:123:0 (checkdoc)    First sentence should end with punctuation
cider-doc.el:129:0 (checkdoc)    First sentence should end with punctuation
cider-doc.el:135:0 (checkdoc)    First sentence should end with punctuation
cider-doc.el:141:0 (checkdoc)    First sentence should end with punctuation
cider-doc.el:191:0 (checkdoc)    First line is not a complete sentence
Found 5 warnings in file ‘cider-doc.el’

cider-popup.el:30:0 (checkdoc)   First sentence should end with punctuation
Found 1 warning in file ‘cider-popup.el’

cider-scratch.el:74:0 (checkdoc) Lisp symbol ‘clojure-mode’ should appear in quotes
Found 1 warning in file ‘cider-scratch.el’

cider-stacktrace.el:89:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:95:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:101:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:107:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:113:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:119:0 (checkdoc) First sentence should end with punctuation
cider-stacktrace.el:125:0 (checkdoc) First sentence should end with punctuation
Found 7 warnings in file ‘cider-stacktrace.el’

is there some linter config I'm missing?

@plexus plexus mentioned this pull request Aug 31, 2021
Open
8 tasks
@bbatsov bbatsov merged commit 28fac9c into clojure-emacs:master Aug 31, 2021
@bbatsov
Copy link
Member

bbatsov commented Aug 31, 2021

Good catch, and good point about doing input sanitization on nREPL's side.

You can ignore the checkdoc warnings.

@plexus plexus mentioned this pull request Aug 31, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plexus @bbatsov
Morty Proxy This is a proxified and sanitized view of the page, visit original site.