Implement index parameter on api/create issue

[guillep2k]

Fixes #7790.

[guillep2k]

Note: I should write an integration test for this. Will do tomorrow. Changed to WIP.

[codecov-io]

Codecov Report

Merging #7876 into master will increase coverage by 0.03%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #7876      +/-   ##
==========================================
+ Coverage   41.43%   41.47%   +0.03%     
==========================================
  Files         478      478              
  Lines       63943    63945       +2     
==========================================
+ Hits        26492    26518      +26     
+ Misses      34001    33981      -20     
+ Partials     3450     3446       -4

Impacted Files	Coverage Δ
routers/api/v1/repo/pull.go	37.43% <ø> (+0.04%)	⬆️
routers/api/v1/repo/issue.go	24.84% <100%> (+1.62%)	⬆️
models/issue.go	50.84% <75%> (+0.88%)	⬆️
models/error.go	33.47% <0%> (+1.08%)	⬆️

---

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 2d0b90c...73afec1. Read the comment docs.

[lunny]

I feel it's dangerous to let user give an index but I haven't read the codes carefully.

[guillep2k]

I feel it's dangerous to let user give an index but I haven't read the codes carefully.

@lunny For what I've seen, there should be no side-effects. Just as @argv-minus-one, the index column is calculated from a query (select max(index)+1) right before inserting the new record in the database. It's not an identity column, as I was confused before. The rest of the code couldn't care less what indexes the issues have. In fact, there is a potential problem with the current implementation: there's a slight chance of a racing condition where two people attempt to add issues the traditional way:

1. User A submits the form creating an issue for repo abc.
2. User B submits the form creating another issue for repo abc.
3. At issue.go, in the context of user A's request, getMaxIndexOfIssue() returns 99. It will be used at step 5.
4. At issue.go, in the context of user B's request, getMaxIndexOfIssue() also returns 99. because at the moment that's the latest issue for the repo. It will be used at step 6.
5. At issue.go, in the context of user A's request, newIssue() inserts A's issue with Index: 100.
6. At issue.go, in the context of user B's request, newIssue() attempts to insert B's issue with Index: 100 and fails, because 100 was used at step 5.

This will of course depend on the database locking model.

And this is for current code. Inserting issues by explicit index will not improve or worsen this situation.

Using the API to insert issues is the user's responsibility. The field is uint64, so there will be no negative values, and a 0 means default. If the user sends a duplicate index, he will get a 500 error response with:

{
    "message": "newIssue: pq: duplicate key value violates unique constraint \"UQE_issue_repo_index\"",
    "url":"http://srep.sif.com.ar:3080/api/swagger"
}

Which they should be expecting, anyway (the actual message could be different for each DB type and version). No intermediate states are kept, so the whole operation fails successfully. 😂

But!

The only "danger" I can think of is if the user creates an issue with index: 2^64-1 (i.e. max value), so Gitea will be unable to create any more indexes. Could it be considered a security problem (potential DoS)? Perhaps we should have the feature disabled by default and the user would set it to enabled in app.ini at their discretion.

[lunny]

@guillep2k step 6 will failed because we have a unique index with repo_id and index on database design. A slight enhancement should be retry it 3 times to retrieve the max id.

Yes, an index number with 2^64-1 will broken the gitea. It's a security problem if we allow customize index number.

[guillep2k]

@guillep2k step 6 will failed because we have a unique index with repo_id and index on database design. A slight enhancement should be retry it 3 times to retrieve the max id.

Yes, an index number with 2^64-1 will broken the gitea. It's a security problem if we allow customize index number.

@lunny The "step 6" problem is independent of this PR. I'll create an issue for it (chances are slim to reproduce this problem in a real-life scenario; even to test it).

The "index == 2^64 - 1" problem is contained by allowing only admins to create issues like this; it would be their responsiblity to avoid that. With my last commit, it won't break anything; it will just prevent creating anymore issues with error "Index numbers depleted".

[guillep2k]

Created separate issue #7887 to handle the racing condition.

[argv-minus-one]

Thanks for doing this!

Some problems I found/thought of:

• The issue number field is named id in the newly-defined JSON API. This is inconsistent; other parts of the JSON API for issues (such as GET /api/v1/repos/<user>/<repo>/issues) put it in the number field instead, and have a separate id field that corresponds to the database id column (that is, the globally-unique issue identifier, not the per-repo issue number).

• The return type of getMaxIndexOfIssue is int64, not uint64. When it overflows, the result is -9223372036854775808, not 0. (Tested on Gitea 1.9.0.)

• Though Gitea and the database can deal with 64-bit issue numbers, JavaScript can't. Integers greater than 2^53-1 (that is, 9007199254740991) are not representable as a JavaScript Number. They are representable as a JavaScript BigInt, but most browsers don't support that, and even in those that do, the JSON parser doesn't generate BigInts.

• The API probably shouldn't allow a negative issue number.

Suggestions:

• Rename the newly defined JSON id field to number, for consistency with the rest of the API.

• When creating an issue, check that the issue number (whether given explicitly or automatically chosen) is >= 0 and <= 0x20000000000000.

---

Another thing: Should non-admin users be allowed to set explicit issue numbers on repositories they own? If I'm reading the code right, only admin users are allowed to do this right now. This limits the usefulness of the feature, since non-admin users can't use it to import their issues without an admin's assistance.

In Gitea 1.9.0, setting an issue number to 2^63-1 in the database blocks creation of new issues on the affected repository, but does not seem to affect the other repository I created in my test instance. Repository owners creating issue #2^63-1 would be blocking only themselves.

Of course, there's the possibility of a repository owner's credentials being stolen, and an attacker creating issue #2^63-1 in order to make issue tracking permanently unusable for that repository. Since deleting issues is not allowed, only someone with direct access to the database can fix that.

This could be solved by allowing repository owners to delete issues on their repositories. Is there a reason why deleting issues isn't currently allowed?

[guillep2k]

* The issue number field is named `id` in the newly-defined JSON API. This is inconsistent; other parts of the JSON API for issues (such as `GET /api/v1/repos/<user>/<repo>/issues`) put it in the `number` field instead, and have a separate `id` field that corresponds to the database `id` column (that is, the globally-unique issue identifier, not the per-repo issue number).

I'll fix that.

* Though Gitea and the database can deal with 64-bit issue numbers, JavaScript can't. Integers greater than 2^53-1 (that is, 9007199254740991) are not representable as a JavaScript `Number`. > 
* The API probably shouldn't allow a negative issue number.

Good catch. I'll fix that too.

Another thing: Should non-admin users be allowed to set explicit issue numbers on repositories they own? If I'm reading the code right, only admin users are allowed to do this right now. This limits the usefulness of the feature, since non-admin users can't use it to import their issues without an admin's assistance.

It's a repository admin (e.g. owner, or member of a team with admin rights for that repo), not necessarily the site admin.

As I understand it, this modification is only useful for a very small group of Gitea users so, the simpler, the better its chances of seeing the light.

[lunny]

@guillep2k Maybe even repositories' admin or owner should not use this API. Only site admin.

@argv-minus-one This is a http/https request, unrelated with Javascript. You can use any language which support int64 to invoke that.

[guillep2k]

This is a http/https request, unrelated with Javascript. You can use any language which support int64 to invoke that.

@lunny perhaps some front-end will fail if Gitea provides a number larger than supported? I'm not familiar with Gitea's JS code. For instance, when the user is commenting in the issue/PR page and types #, 9, 9 ... a list of issues starting with #99.. will pop-up. If the indices are too big to handle, that function will choke with them.

In my latest commit I allow index == 2^63-1 (max int64), but in the one before it I've set it to 2^53-1 if you want to take a look. We could implement it either way.

[guillep2k]

In fact router/api/v1/repo/pull.go is calculating issue.Index by it's own, but that value should not be used. It was being ignored originally; it was just dead code. I incorrectly accepted the value. Instead of accepting it when it comes from a PR, I'll remove the PR's dead code, as I should have.

[guillep2k]

@lunny I made one change after your approval. Please review again.

[guillep2k]

Having become a bit of an expert on Issue.Index in the past few days, I now think that this PR should not be merged. My reasoning is as follows:

• If it's for some kind of one-time migration, it should be implemented separately in the migration package, as suggested at some point.
• If it's for continuous usage, then it will create a problem. Say the admin adds 232 issues from an external source: 1001 through 1232. Then someone uses Gitea's UI to file a new issue, which is assigned with index 1233. The next time the admin tries to migrate issues from the external source, 1233 will have been taken, and it could be clashing with the index number they're trying to create.

So, I think this is a no-win in all cases.

[lunny]

Let's close then.