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

strictVerify types seem wrong #344

Open
t3chguy opened this issue Feb 3, 2025 · 8 comments · May be fixed by #345
Open

strictVerify types seem wrong #344

t3chguy opened this issue Feb 3, 2025 · 8 comments · May be fixed by #345

Comments

@t3chguy
Copy link

t3chguy commented Feb 3, 2025

The type is an optional boolean, yet the code has handling for string values.

osx-sign/src/sign.ts

Lines 78 to 85 in 757fa60

opts.strictVerify !== false && compareVersion(osRelease, '15.0.0') >= 0 // Strict flag since darwin 15.0.0 --> OS X 10.11.0 El Capitan
? [
'--strict' +
(opts.strictVerify
? '=' + opts.strictVerify // Array should be converted to a comma separated string
: '')
]
: [],

Worse yet, treating it as a boolean entirely fails in my experience.

  failedTask=build stackTrace=Error: Command failed: codesign --verify --deep --strict=true --verbose=2 /Users/runner/work/element-desktop/element-desktop/dist/mac-universal/Element Nightly.app
invalid strict option - true

Indeed the man page does not list --strict=true as a valid option.

     --strict options
             When validating code, apply additional restrictions beyond the defaults.

             symlinks  Check that symbolic links inside the code bundle point to sealed files inside its bundle.  This means that broken symbolic links are rejected, as are links
                       to places outside the bundle and to places that are not, for whatever reason, sealed by the signature.

             sideband  Check that no resource forks, Finder attributes, or similar sideband data is present in the signed code.  This is now automatically enforced by signing
                       operations.
             Options can be specified as a comma-separated list. Use plain --strict or --strict=all to be as strict as possible. Note that --strict=all may include more checking
             types over time.
             Not all strictness check make sense in all circumstances, which is why these behaviors are not the defualt.
@t3chguy
Copy link
Author

t3chguy commented Feb 3, 2025

Additionally misleading as it says the defaultValue is true - and the behaviour indeed provides --strict but if you actually provide a value of true then the behaviour changes entirely.

Looks like the README when the option was added was correct af53ad0#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R214

`strictVerify` - *Boolean|String|Array.<String>*

@t3chguy t3chguy mentioned this issue Feb 3, 2025
Open
@mmaietta
Copy link

mmaietta commented Feb 3, 2025

I'm not sure I follow. So what are the values supposed to be for strictVerify? That configuration code hasn't changed in years. If true is provided, don't pass in strictVerify: true, only if it's set to false?

@t3chguy
Copy link
Author

t3chguy commented Feb 3, 2025 •
edited
Loading

So what are the values supposed to be for strictVerify

Given osx-sign just passes the value to --strict=<value> verbatim for any non-false values the codesign man page (copied in the OP) says it can be

"symlinks" | "sideband" | "all" | "symlinks,sideband" | "sideband,symlinks" though string would probably be a fair analogue.

Or if osx-sign wants to only support on/off boolean behaviour then the code needs to be updated to correctly handle a true value so that it passes --strict rather than the invalid --strict=true. Alternatively the type could be updated to strictVerify?: false; given only the false value works as documented (and undefined/default would then be --strict).

@mmaietta
Copy link

mmaietta commented Feb 3, 2025 •
edited
Loading

@erickzhao , what are your thoughts? Can we update the typescript definition?

osx-sign/src/types.ts

Lines 175 to 180 in 757fa60

/**
* Flag to enable/disable the `--strict` flag when verifying the signed application bundle.
*
* @defaultValue `true`
*/
strictVerify?: boolean;

Seems like it should be:

    strictVerify?: false | "symlinks" | "sideband" | "all" | "symlinks,sideband" | "sideband,symlinks";

It'd be a major semver update, so it could be timed with the node 22 upgrade. Alternatively, a new config prop strict is created with strictVerify deprecated (for now), but then there's a migration path (presumably node 22 upgrade could then drop it?)

@t3chguy
Copy link
Author

t3chguy commented Feb 3, 2025 •
edited
Loading

I'd say to avoid maintenance burden maybe just strictVerify?: false | string; would be more sane, that way if codesign adds other strict options this pkg won't need updating, it'd just work. Or for backwards-compat: strictVerify?: boolean | string; - this is the type described in the README when strictVerify was added + fixing strictVerify=true to not wrongly add =true in the command execution

@mmaietta
Copy link

mmaietta commented Feb 3, 2025

The issue imo with a blank string is that it provides perhaps-too-little guidance for the user (compared to a vanilla true/false). If a new strict option were added, it's just a patch version bump I'd imagine.

@t3chguy t3chguy linked a pull request Feb 3, 2025 that will close this issue
Open
@t3chguy
Copy link
Author

t3chguy commented Feb 3, 2025

Even just fixing the boolean behaviour is good enough for me, I think relying on the default value as the only working true is really strange. I have #345 which does both fix boolean behaviour and add a string behaviour as an advanced mode

@erickzhao erickzhao added bug and removed bug labels Feb 3, 2025
@erickzhao
Copy link
Member

I think this is sensible. If the previous TypeScript definitions were broken, I think changing these to something that works wouldn't be a breaking change regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: strictVerify not working with true value t3chguy/osx-sign
3 participants
@t3chguy @mmaietta @erickzhao