Skip to content

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Feb 27, 2025

See commit description for more.

Some relevant PRs:

#5819
#5807
#5076

cc @thoughtpolice @neongreen @martinvonz

See https://github.com/jj-vcs/jj/actions/runs/13559508098/job/37899985506 . The GH_PAGES variable is populated, but we get the same error.

@jj-vcs jj-vcs deleted a comment from gemini-code-assist bot Feb 27, 2025
@ilyagr ilyagr marked this pull request as ready for review February 27, 2025 05:30
@ilyagr ilyagr enabled auto-merge February 27, 2025 05:30
@ilyagr ilyagr disabled auto-merge February 27, 2025 05:34
@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 27, 2025

I think I figured out what the problem actually is. It's persist-credetials: false in the workflow in the checkout action. I'm not sure what the correct way to get around that is except for setting persist-credentials: true.

I won't merge this until I research it a bit more. For example, will fixing persist-credentials upset zizmor?

@ilyagr ilyagr marked this pull request as draft February 27, 2025 05:36
@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 27, 2025

Yes, I think this is a proper fix now. I think the security implications are OK, but if there's doubt, we don't have to rush to merge this. (Though we should figure something out a few days before the upcoming release in a week)

@ilyagr ilyagr marked this pull request as ready for review February 27, 2025 05:44
@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 27, 2025

FYI, this is how the checkout action sets up git auth and (currently, with persist-credentials:false) removes it:

image

You need to unfold the appropriate triangles in https://github.com/jj-vcs/jj/actions/runs/13559508098/job/37899985506 to see this.

@ilyagr ilyagr force-pushed the ig/docsfix2 branch 2 times, most recently from 20d22e7 to 731bd2b Compare February 27, 2025 06:07
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Sure, let's try it

I think I likely found the issue. zizmor seems OK
with persisting credentials, see
https://github.com/jj-vcs/jj/actions/runs/13559693565/job/37900455060?pr=5820

Both of these workflows run only on commits in `main`,
so this doesn't seem like a huge security hole, but
we could consider other, better solutions in the
future.

Follow up to 78177ff. See #5819 for a failed attempt.
cc @thoughtpolice @neongreen @martinvonz
@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 27, 2025

I added one more comment here. I'll probably still start the merge process soon, but feel free to cancel it and let me know if you don't like it. The comment is:

# IMPORTANT: this workflow also functions as a test for `docs-deploy-website-latest-release` in
# releases.yml. Any fixes here should probably be duplicated there.

@ilyagr ilyagr enabled auto-merge February 27, 2025 06:18
@ilyagr ilyagr added this pull request to the merge queue Feb 27, 2025
Merged via the queue into main with commit f399c57 Feb 27, 2025
45 checks passed
@ilyagr ilyagr deleted the ig/docsfix2 branch February 27, 2025 06:47
@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 27, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants