Use GIT_EDITOR for editing and approving with cargo vet certify #180

mystor · 2022-06-14T14:27:19Z

This is an alternative approach to the vet certify UX which uses the GIT_EDITOR in order to prompt and ask the user to input notes, and accept a statement while also showing the criteria descriptions.

In order to function more reliably on Windows, this code attempts to locate the git-for-windows install on the user's machine, and invokes the user's GIT_EDITOR through the git-for-windows' shell environment.

This does not attempt to replace the interactive prompt for selecting which criteria to certify for.

Fixes #165

This is an alternative approach to the `vet certify` UX which uses the GIT_EDITOR in order to prompt and ask the user to input notes, and accept a statement while also showing the criteria descriptions. In order to function more reliably on Windows, this code attempts to locate the git-for-windows install on the user's machine, and invokes the user's GIT_EDITOR through the git-for-windows' shell environment. This does not attempt to replace the interactive prompt for selecting which criteria to certify for. Fixes mozilla#165

mystor · 2022-06-14T16:06:22Z

Example text file for `cargo vet certify indexmap 1.8.1` in the test project:

# Please read the following criteria and uncomment the statement below:

# === BEGIN CRITERIA "safe-to-deploy" ===
#
# This crate will not introduce a serious security vulnerability to production
# software exposed to untrusted input.
#
# Auditors are not required to perform a full logic review of the entire crate.
# Rather, they must review enough to fully reason about the behavior of all unsafe
# blocks and usage of powerful imports. For any reasonable usage of the crate in
# real-world software, an attacker must not be able to manipulate the runtime
# behavior of these sections in an exploitable or surprising way.
#
# Ideally, all unsafe code is fully sound, and ambient capabilities (e.g.
# filesystem access) are hardened against manipulation and consistent with the
# advertised behavior of the crate. However, some discretion is permitted. In such
# cases, the nature of the discretion should be recorded in the `notes` field of
# the audit record.
#
# For crates which generate deployed code (e.g. build dependencies or procedural
# macros), reasonable usage of the crate should output code which meets the above
# criteria.
#
# === END CRITERIA ===
#
# STATEMENT:

# I, Nika Layzell, certify that I have audited version 1.8.1 of indexmap in accordance with the above criteria.

# NOTES:

Gankra

this all looks great!

Gankra · 2022-06-14T16:24:35Z

src/editor.rs

+ }
+
+ // Trim off excess whitespace.
+ Ok(result.trim().to_owned())


i love this code because you trim as much as i do and it is such a relief

I trim perhaps a bit more often than I should, but it seems to work here :p

Gankra · 2022-06-14T16:27:40Z

src/editor.rs

+ assert!(
+ !line.starts_with('#'),
+ "Non-comment lines cannot start with a '#' comment character"
+ );


Hmm this is an interesting failure mode. I was initially worried this would create roundtripping issues but we never feed existing notes from a toml back into the editor, so the only weird case would be someone passing notes in on the CLI with an explicit # line and they'll get a very quick error so seems fine.

Yeah, I didn't love this as a failure case either. In git there's support for trying a sequence of comment characters and only falling over if all are used at the same time (https:/git/git/blob/9c897eef06347cc5a3eb07c3ae409970ab1052c8/builtin/commit.c#L669-L697), which I could add but it would require some changes to buffer the file in memory and only write it out with comments once it's been fully written.

The only place where this can come up right now though is if you pass a note on the command line which starts with a # character on some line, as we write the note from the command line out in the file for editing.

Gankra · 2022-06-14T16:29:24Z

src/editor.rs

+ let file = BufReader::new(File::open(&path)?);
+ for line in file.lines() {
+ let line = line?;
+ if line.starts_with('#') {


confirming: a line that starts with a space and then a # is indeed not supposed to be treated as a comment? (both this code and add_comments seem to implicitly have this rule.

That said this is kind of weird because if you have leading space on a line we will trim it away afterwards, so that leading space being significant is strange...

I suppose relatedly, do we want someone to be able to have any kind of indentation in their notes? Because this code will absolutely just eat that indentation.

I think this code will only eat indentation at the start or end of the notes. IIRC I don't trim individual lines at all, so you can have leading whitespace for lines in the middle of your note.

The logic for comments is roughly copied from git's commitmessage, where IIRC only comment characters at the very start of the line are considered: https:/git/git/blob/74cc1aa55f30ed76424a0e7226ab519aa6265061/strbuf.c#L1135. I don't do all of the other fancy line cleanup stuff like removing trailing whitespace or collapsing extra newlines though :-)

You immediately trim the line just below: result.push_str(line.trim());

Oops, you're right. My bad

Gankra · 2022-06-14T16:44:40Z

src/editor.rs

+const LINE_ENDING: &str = "\r\n";
+
+#[cfg(not(windows))]
+const LINE_ENDING: &str = "\n";


NB Git does have some settings related to line endings and they are very annoying (this is fine for now though, especially since they're purely transient afaict) rust-lang/cargo#8523

Yes, I agree that git's work around newlines is very annoying. This was basically just added as a defensive measure for when opening the editor on Windows, as the windows newline has no risk of being preserved, and everything's normalized immediately after. If I didn't do this, I believe that the notepad fallback on older windows systems would break, because notepad didn't support unix line endings for the longest time.

Gankra · 2022-06-14T17:02:59Z

src/editor.rs

+// NOTE: This is probably not as reliably available as `vi`, but is easier to
+// quit from for users who aren't familiar with vi.
+#[cfg(not(windows))]
+const FALLBACK_EDITOR: &str = "nano";


mystor force-pushed the certify_editor branch from a30bb75 to 1bb7c90 Compare June 14, 2022 15:17

mystor mentioned this pull request Jun 14, 2022

investigate whether we should be using libgit2 over git-as-a-binary #175

Closed

mystor force-pushed the certify_editor branch from 1bb7c90 to c12dd96 Compare June 14, 2022 15:40

mystor marked this pull request as ready for review June 14, 2022 15:46

Gankra approved these changes Jun 14, 2022

View reviewed changes

Handle whitespace in edited notes better

d1a8bb8

mystor merged commit 24d8d8d into mozilla:main Jun 14, 2022

Gankra mentioned this pull request Jun 14, 2022

certify should explicitly prompt for notes #144

Closed

mystor mentioned this pull request Jun 15, 2022

Update book for certify UX changes #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GIT_EDITOR for editing and approving with cargo vet certify #180

Use GIT_EDITOR for editing and approving with cargo vet certify #180

mystor commented Jun 14, 2022 •

edited

Loading

mystor commented Jun 14, 2022

Gankra left a comment

Gankra Jun 14, 2022

mystor Jun 14, 2022

Gankra Jun 14, 2022

mystor Jun 14, 2022

Gankra Jun 14, 2022

Gankra Jun 14, 2022

mystor Jun 14, 2022

Gankra Jun 14, 2022

mystor Jun 14, 2022

Gankra Jun 14, 2022

mystor Jun 14, 2022

Gankra Jun 14, 2022

Use GIT_EDITOR for editing and approving with cargo vet certify #180

Use GIT_EDITOR for editing and approving with cargo vet certify #180

Conversation

mystor commented Jun 14, 2022 • edited Loading

mystor commented Jun 14, 2022

Gankra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystor commented Jun 14, 2022 •

edited

Loading