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.

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

Fix author links in the proposals lists #1997

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Nov 14, 2024

Links were broken since the upgrade of the hugo version in #1984.

The broken state can be viewed at the preview from that PR. It cannot be viewed at https://spec.matrix.org/proposals yet because it hasn't been updated since the upgrade (although it is broken on a different way because of the upgrade too). Now visible at https://spec.matrix.org/proposals/#work-in-progress

Pull Request Checklist

Preview: https://pr1997--matrix-spec-previews.netlify.app

Links were broken since the upgrade of the hugo version.

Signed-off-by: Kévin Commaille <[email protected]>
@zecakeh zecakeh requested a review from a team as a code owner November 14, 2024 15:27
Signed-off-by: Kévin Commaille <[email protected]>
Comment on lines 63 to 64
{{ $authors_list := apply .authors "htmlEscape" "." }}
{{ $authors_list := apply .authors "printf" "<a href=\"https://github.com/%s\">@%s</a>" "." "." }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this. Isn't the new value immediately overrwritten?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, it's using the wrong variable here. The htmlEscape is a only precaution anyway in case there is a special HTML character in the username of an author, but I'm not even sure GitHub would allow that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember now why I did that, even if GitHub doesn't allow special characters in the username, it's because the data from the MSC description is not validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ad now I realized that for the URL, we should not use htmlEscape but rather a function that converts to a URL-compatible format. Hugo has urlize but it also transforms the string instead of just converting special characters.

Maybe we should consider sanitizing to be the job of the script that fetches the proposals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the call to htmlEscape, maybe it can be done separately, to see if we want to sanitize the input of the proposals.

@richvdh
Copy link
Member

richvdh commented Nov 19, 2024

Could you explain how #1984 broke this?

@zecakeh
Copy link
Contributor Author

zecakeh commented Nov 19, 2024

Could you explain how #1984 broke this?

I am not sure but there was a Hugo update, so I guess a fix on their part requires the use of safeHTML now, which makes sense in a place where HTML is expected and Hugo cannot be sure that the content is really HTML given that it's constructed from a string.

Signed-off-by: Kévin Commaille <[email protected]>
@zecakeh zecakeh requested a review from richvdh November 20, 2024 10:17
@richvdh richvdh changed the title Fix the proposals lists Fix author links in the proposals lists Nov 20, 2024
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thanks!

@richvdh richvdh merged commit 69e162e into matrix-org:main Nov 20, 2024
12 checks passed
@zecakeh zecakeh deleted the fix-proposals branch November 20, 2024 15:51
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