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

Prevent NPE on partial match of compare URL and allow short SHA1 compare URLs #18472

Merged
merged 3 commits into from
Jan 30, 2022
Merged

Prevent NPE on partial match of compare URL and allow short SHA1 compare URLs #18472

merged 3 commits into from
Jan 30, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Jan 30, 2022 •
edited by zeripath
Loading

  • Prevent NPE panic when the full compare url regex isn't matched
  • Allow the usage of a shorter sha1 being used.

Fix #18471

Don't panic & allow shorter sha1
21442e8 
- Don't panic when the full regex isn't matched and allow the usage of a
shorter sha1 being used.
- Resolves go-gitea#18471
@Gusted Gusted force-pushed the fix-markdown-error branch from 8adabdd to 21442e8 Compare January 30, 2022 19:56
Gusted pushed a commit to Gusted/gitea that referenced this pull request Jan 30, 2022
Don't panic & allow shorter sha1 (go-gitea#18472)
b10b279 
- Backport of go-gitea#18472
@Gusted Gusted mentioned this pull request Jan 30, 2022
Merged
@Gusted Gusted added outdated/theme/markdown backport/done All backports for this PR have been created labels Jan 30, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jan 30, 2022
@Gusted Gusted changed the title Don't panic & allow shorter sha1 Don't panic & allow shorter sha1 on compare URL Jan 30, 2022
zeripath
zeripath reviewed Jan 30, 2022
View reviewed changes
modules/markup/html.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 30, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 30, 2022
@zeripath zeripath changed the title Don't panic & allow shorter sha1 on compare URL Prevent NPE on partial match of compare URL and allow short SHA1 compare urls Jan 30, 2022
@zeripath zeripath changed the title Prevent NPE on partial match of compare URL and allow short SHA1 compare urls Prevent NPE on partial match of compare URL and allow short SHA1 compare URLs Jan 30, 2022
@zeripath
Copy link
Contributor

Regression #17924

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2022 •
edited
Loading

Codecov Report

Merging #18472 (f36409a) into main (b2250d2) will increase coverage by 0.00%.
The diff coverage is 25.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18472   +/-   ##
=======================================
  Coverage   46.26%   46.27%           
=======================================
  Files         842      842           
  Lines      121181   121185    +4     
=======================================
+ Hits        56068    56073    +5     
+ Misses      58307    58299    -8     
- Partials     6806     6813    +7
Impacted Files Coverage Δ
modules/markup/html.go 79.53% <25.00%> (-0.27%) ⬇️
modules/git/repo_base_nogogit.go 70.58% <0.00%> (-3.93%) ⬇️
modules/git/utils.go 66.29% <0.00%> (-3.38%) ⬇️
models/unit/unit.go 46.90% <0.00%> (-1.77%) ⬇️
modules/queue/workerpool.go 55.05% <0.00%> (-1.13%) ⬇️
models/issue_comment.go 51.31% <0.00%> (-0.69%) ⬇️
modules/queue/queue_channel.go 81.13% <0.00%> (+2.83%) ⬆️
modules/queue/queue_disk.go 63.33% <0.00%> (+3.33%) ⬆️
modules/queue/queue_bytefifo.go 55.35% <0.00%> (+5.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2250d2...f36409a. Read the comment docs.

@zeripath
Copy link
Contributor

Oh actually @Gusted it would be good to add a test case for something where there would only be a partial match

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 30, 2022
@lafriks lafriks merged commit 72256c1 into go-gitea:main Jan 30, 2022
lafriks pushed a commit that referenced this pull request Jan 30, 2022
@zeripath
Prevent NPE on partial match of compare URL and allow short SHA1 comp…
d5027b6 
…are URLs (#18472) (#18473)

* Don't panic & allow shorter sha1 (#18472)

- Backport of #18472

* Improve comment

Co-authored-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this pull request Jan 31, 2022
@zeripath
Prevent NPE on partial match of compare URL (part 2)
d4dfa50 
Unfortunately go-gitea#18472 only fixed part of the ways a nil pointer can occur in the
compare url. We also need to ensure that the match array has all the matches present.

Fix go-gitea#18471
Related go-gitea#18472

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Jan 31, 2022
Closed
zeripath added a commit to zeripath/gitea that referenced this pull request Jan 31, 2022
@zeripath
Prevent NPE on partial match of compare URL (part 2) (go-gitea#18490)
2a238f8 
Backport go-gitea#18490

Unfortunately go-gitea#18472 only fixed part of the ways a nil pointer can occur in the
compare url. We also need to ensure that the match array has all the matches present.

Fix go-gitea#18471
Related go-gitea#18472

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Jan 31, 2022
Closed
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@zeripath @techknowlogick
Prevent NPE on partial match of compare URL and allow short SHA1 comp…
eea6533 
…are URLs (go-gitea#18472)

* Don't panic & allow shorter sha1

- Don't panic when the full regex isn't matched and allow the usage of a
shorter sha1 being used.
- Resolves go-gitea#18471

* Update modules/markup/html.go

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/theme/markdown type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Gitea 1.16.0 crash when click on repo link
6 participants
@Gusted @zeripath @codecov-commenter @techknowlogick @lafriks @GiteaBot