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

Adds SecurityDetails.subjectAlternativeNames #2820

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Adds SecurityDetails.subjectAlternativeNames #2820

merged 2 commits into from
Nov 7, 2024

Conversation

jpeirson
Copy link
Contributor

@jpeirson jpeirson commented Nov 6, 2024

which is part of the Puppeteer API but not yet in PuppeterSharp

Adds SecurityDetails.subjectAlternativeNames
3d80d0f 
which is part of the Puppeteer API but not yet in PuppeterSharp
@jpeirson
Copy link
Contributor Author

jpeirson commented Nov 6, 2024

I didn't see any existing tests around SecurityDetails so I didn't extend any unit tests. I can add a new test fixture if appropriate.

Also, it looks like the CDP JSON payload to SecurityDetails mapping happens via some System.Text.Json automagic? So I'm assuming there are no code changes to make to actually populate the new SubjectAlternativeNames.

@jpeirson
Copy link
Contributor Author

jpeirson commented Nov 6, 2024

Three tests failed, but looks like unrelated to my change?

@jpeirson jpeirson marked this pull request as ready for review November 6, 2024 21:58
kblok
kblok approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@kblok kblok left a comment

Choose a reason for hiding this comment

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

It looks good!

Is this a blocker for you? I you want to ship this. Update the patch version in the csproj file.

@jpeirson
Copy link
Contributor Author

jpeirson commented Nov 7, 2024

Bumped version to 20.0.5

The missing SANs property was relatively minor for me - I could still resort to Network.getCertificate if I had to - but I saw that pptr supports it so might as well support the change.

@kblok kblok merged commit c908c5c into hardkoded:master Nov 7, 2024
9 of 12 checks passed
@jpeirson jpeirson deleted the feature/securitydetails-sans branch November 7, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kblok kblok kblok approved these changes

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
@jpeirson @kblok