feat: /project/reorder + /project/create#19
Merged
Merged
Conversation
There was a problem hiding this comment.
Hey - 我发现了 2 个问题,并留下了一些高层次的反馈:
- 在
/project/reorder/{secret}和/project/create/{secret}这两个接口中,如果admin_secret未设置或为空,当前的admin_secret校验会静默拒绝所有调用;建议在服务启动时快速失败,或者记录清晰的配置错误日志,而不是在这种情况下返回通用的 403。 - reorder 接口目前会静默过滤掉未知的
rid;建议将req.rid_list与db_rids做校验,并在出现意外或重复 ID 时返回 4xx 错误(或至少记录日志),这样可以更容易发现客户端的错误。 - 在
create_project中调用Project.create时,没有检查是否已经存在具有相同rid的项目;建议在模型层强制唯一性,或者显式处理冲突,以避免造成让人困惑的部分失败。
供 AI Agents 使用的提示词
Please address the comments from this code review:
## Overall Comments
- In both `/project/reorder/{secret}` and `/project/create/{secret}`, the `admin_secret` check will silently reject all calls if `admin_secret` is unset or empty; consider failing fast at startup or logging a clear configuration error instead of returning a generic 403 in this case.
- The reorder endpoint currently filters unknown `rid`s out silently; consider validating `req.rid_list` against `db_rids` and returning a 4xx error (or at least logging) when unexpected or duplicate IDs are provided to make client mistakes easier to detect.
- In `create_project`, `Project.create` is called without checking whether a project with the same `rid` already exists; consider either enforcing uniqueness at the model level or explicitly handling collisions to avoid confusing partial failures.
## Individual Comments
### Comment 1
<location path="src/project/__init__.py" line_range="75-80" />
<code_context>
+ all_projects = list(Project.select())
+ db_rids = {p.rid for p in all_projects}
+
+ ordered_rids = [rid for rid in req.rid_list if rid in db_rids]
+ remaining_rids = [p.rid for p in sorted(all_projects, key=lambda p: p.proj_index) if p.rid not in set(ordered_rids)]
+ final_order = ordered_rids + remaining_rids
+
</code_context>
<issue_to_address>
**suggestion:** Guard against duplicate rids in the reorder request to avoid assigning multiple indices to the same project.
If `req.rid_list` contains duplicates, the same `rid` will appear multiple times in `final_order`, so its `proj_index` is reassigned several times. While the end state is consistent, it makes the ordering logic harder to reason about and debug. Consider deduplicating `req.rid_list` while preserving first-seen order (e.g., building `ordered_rids` via an ordered-set pattern) so each project is assigned exactly one index.
```suggestion
all_projects = list(Project.select())
db_rids = {p.rid for p in all_projects}
seen_rids = set()
ordered_rids = []
for rid in req.rid_list:
if rid in db_rids and rid not in seen_rids:
seen_rids.add(rid)
ordered_rids.append(rid)
remaining_rids = [
p.rid
for p in sorted(all_projects, key=lambda p: p.proj_index)
if p.rid not in seen_rids
]
final_order = ordered_rids + remaining_rids
```
</issue_to_address>
### Comment 2
<location path="src/project/__init__.py" line_range="92" />
<code_context>
+ return {"ec": 200, "msg": "ok"}
+
+
+@router.post("/project/create/{secret}")
+async def create_project(secret: str, req: CreateProjectRequest):
+ if not settings.admin_secret or secret != settings.admin_secret:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Embedding the admin secret in the URL path can leak it via logs and intermediaries.
Because `secret` is in the path, it will end up in server/reverse-proxy logs and possibly monitoring tools, increasing the likelihood of exposure. Prefer sending this secret in an HTTP header or request body field, where you can more easily control and scrub it from logs while preserving the same authorization behavior.
</issue_to_address>帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据这些反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In both
/project/reorder/{secret}and/project/create/{secret}, theadmin_secretcheck will silently reject all calls ifadmin_secretis unset or empty; consider failing fast at startup or logging a clear configuration error instead of returning a generic 403 in this case. - The reorder endpoint currently filters unknown
rids out silently; consider validatingreq.rid_listagainstdb_ridsand returning a 4xx error (or at least logging) when unexpected or duplicate IDs are provided to make client mistakes easier to detect. - In
create_project,Project.createis called without checking whether a project with the sameridalready exists; consider either enforcing uniqueness at the model level or explicitly handling collisions to avoid confusing partial failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `/project/reorder/{secret}` and `/project/create/{secret}`, the `admin_secret` check will silently reject all calls if `admin_secret` is unset or empty; consider failing fast at startup or logging a clear configuration error instead of returning a generic 403 in this case.
- The reorder endpoint currently filters unknown `rid`s out silently; consider validating `req.rid_list` against `db_rids` and returning a 4xx error (or at least logging) when unexpected or duplicate IDs are provided to make client mistakes easier to detect.
- In `create_project`, `Project.create` is called without checking whether a project with the same `rid` already exists; consider either enforcing uniqueness at the model level or explicitly handling collisions to avoid confusing partial failures.
## Individual Comments
### Comment 1
<location path="src/project/__init__.py" line_range="75-80" />
<code_context>
+ all_projects = list(Project.select())
+ db_rids = {p.rid for p in all_projects}
+
+ ordered_rids = [rid for rid in req.rid_list if rid in db_rids]
+ remaining_rids = [p.rid for p in sorted(all_projects, key=lambda p: p.proj_index) if p.rid not in set(ordered_rids)]
+ final_order = ordered_rids + remaining_rids
+
</code_context>
<issue_to_address>
**suggestion:** Guard against duplicate rids in the reorder request to avoid assigning multiple indices to the same project.
If `req.rid_list` contains duplicates, the same `rid` will appear multiple times in `final_order`, so its `proj_index` is reassigned several times. While the end state is consistent, it makes the ordering logic harder to reason about and debug. Consider deduplicating `req.rid_list` while preserving first-seen order (e.g., building `ordered_rids` via an ordered-set pattern) so each project is assigned exactly one index.
```suggestion
all_projects = list(Project.select())
db_rids = {p.rid for p in all_projects}
seen_rids = set()
ordered_rids = []
for rid in req.rid_list:
if rid in db_rids and rid not in seen_rids:
seen_rids.add(rid)
ordered_rids.append(rid)
remaining_rids = [
p.rid
for p in sorted(all_projects, key=lambda p: p.proj_index)
if p.rid not in seen_rids
]
final_order = ordered_rids + remaining_rids
```
</issue_to_address>
### Comment 2
<location path="src/project/__init__.py" line_range="92" />
<code_context>
+ return {"ec": 200, "msg": "ok"}
+
+
+@router.post("/project/create/{secret}")
+async def create_project(secret: str, req: CreateProjectRequest):
+ if not settings.admin_secret or secret != settings.admin_secret:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Embedding the admin secret in the URL path can leak it via logs and intermediaries.
Because `secret` is in the path, it will end up in server/reverse-proxy logs and possibly monitoring tools, increasing the likelihood of exposure. Prefer sending this secret in an HTTP header or request body field, where you can more easily control and scrub it from logs while preserving the same authorization behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
添加经过认证的管理员端点,用于管理项目排序和创建,并扩展配置以支持管理员密钥。
New Features:
POST /project/reorder端点,用于更新现有项目的显示顺序。POST /project/create端点,用于创建带有可配置元数据的新项目。Enhancements:
admin_secret值,用于授权敏感的项目管理操作。Original summary in English
Summary by Sourcery
Add authenticated admin endpoints for managing project ordering and creation while extending configuration to support an admin secret.
New Features:
Enhancements: