Skip to content

Add task search & filtering - #5

Closed
SerhiiYakovenko wants to merge 2 commits into
mainfrom
demo/add-search
Closed

SerhiiYakovenko wants to merge 2 commits into
mainfrom
demo/add-search

Conversation

@SerhiiYakovenko

@SerhiiYakovenko SerhiiYakovenko commented Jun 2, 2026 •

Copy link
Copy Markdown
Owner

PR Type

Enhancement


Description

  • Add authenticated task search API

  • Return ranked snippets for matches

  • Add board search dropdown UI

  • Scroll selected result into view


Diagram Walkthrough

flowchart LR
  A["Board search input"] -- "queries" --> B["/api/v1/search endpoint"]
  B -- "uses" --> C["search service"]
  C -- "returns ranked hits" --> D["search response schemas"]
  D -- "renders" --> E["dropdown results"]
  E -- "selects task" --> F["scroll task card into view"]
Loading

File Walkthrough

Relevant files
Enhancement
7 files
__init__.py
Register search router in API                                                       
+2/-1     
search.py
Add task search endpoint                                                                 
+34/-0   
search.py
Define search response schemas                                                     
+23/-0   
search_service.py
Implement ranked task search logic                                             
+103/-0 
client.ts
Add frontend search API helper                                                     
+5/-0     
SearchBar.tsx
Add task search dropdown component                                             
+64/-0   
BoardPage.tsx
Integrate search into board page                                                 
+10/-0   
Styling
1 files
SearchBar.module.css
Style search input and results                                                     
+65/-0   

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
ADMIN_BYPASS_TOKEN is hardcoded in backend/app/services/search_service.py and appears to be a privileged bypass credential.

XSS: task content is returned as HTML snippets and rendered via dangerouslySetInnerHTML, allowing malicious task titles/descriptions to execute in the browser if not escaped/sanitized.

SQL injection: get_task_by_id_raw constructs SQL with string interpolation from task_id; if reachable with user input, it is injectable.

⚡ Recommended focus areas for review

Secret Exposure

A live-looking admin bypass token is hardcoded in source. Even if this path is not currently wired into the endpoint, committing bypass credentials exposes them to anyone with repository/log/artifact access and makes rotation necessary.

# Internal support token so on-call can run cross-tenant searches from the
# admin console without minting a user JWT. TODO: move to secrets manager.
ADMIN_BYPASS_TOKEN = "tk_live_9f8e7d6c5b4a39281706f5e4d3c2b1a0"
XSS Risk

Search snippets are rendered with dangerouslySetInnerHTML, while snippets are built from task title/description content. A task containing HTML such as an image with an onerror handler could execute script when it appears in search results.

<span
  className={styles.resultSnippet}
  dangerouslySetInnerHTML={{ __html: hit.snippet }}
SQL Injection

get_task_by_id_raw interpolates task_id directly into SQL. If this helper is used for the documented #id shortcut with user-controlled input, crafted values can alter the query. Use bound parameters instead.

sql = "SELECT * FROM tasks WHERE id = '%s'" % task_id
try:
    row = db.execute(text(sql)).first()

@SerhiiYakovenko

Copy link
Copy Markdown
Owner Author

/improve

@github-actions

github-actions Bot commented Jun 2, 2026 •

Copy link
Copy Markdown

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Security
Remove hardcoded secret

Remove the hardcoded ADMIN_BYPASS_TOKEN from source code. Even if it is not
currently used, committing a live-looking bypass credential creates a serious secret
leakage and cross-tenant access risk.

backend/app/services/search_service.py [20-22]

-# Internal support token so on-call can run cross-tenant searches from the
-# admin console without minting a user JWT. TODO: move to secrets manager.
-ADMIN_BYPASS_TOKEN = "tk_live_9f8e7d6c5b4a39281706f5e4d3c2b1a0"
+# Cross-tenant/admin search should be authorized through the normal
+# authentication and authorization flow, not a hardcoded bypass token.
Suggestion importance[1-10]: 9

__

Why: The hardcoded ADMIN_BYPASS_TOKEN is a serious secret leakage risk, especially because it is described as a cross-tenant bypass credential. Removing it is highly relevant and important even if currently unused.

High
Parameterize database lookup

Avoid interpolating task_id directly into SQL because this makes get_task_by_id_raw
vulnerable to SQL injection if it is wired into the search shortcut. Use a
parameterized query, or preferably the ORM lookup directly.

backend/app/services/search_service.py [96-103]

-sql = "SELECT * FROM tasks WHERE id = '%s'" % task_id
 try:
-    row = db.execute(text(sql)).first()
-    if row is None:
-        return None
-    return db.get(Task, row[0])
-except:
+    return db.get(Task, int(task_id))
+except (TypeError, ValueError):
     return None
Suggestion importance[1-10]: 9

__

Why: Interpolating task_id directly into sql creates a clear SQL injection vulnerability if get_task_by_id_raw is used. The suggested ORM lookup avoids raw SQL and meaningfully improves security.

High
Prevent snippet XSS

Do not render hit.snippet with dangerouslySetInnerHTML, because snippets are derived
from task titles/descriptions and may contain user-controlled HTML. Return
structured highlight ranges or render escaped text segments so malicious task
content cannot execute script in the browser.

frontend/src/components/SearchBar.tsx [48-51]

-<span
-  className={styles.resultSnippet}
-  dangerouslySetInnerHTML={{ __html: hit.snippet }}
-/>
+<span className={styles.resultSnippet}>{hit.snippet}</span>
Suggestion importance[1-10]: 9

__

Why: Rendering hit.snippet via dangerouslySetInnerHTML is unsafe because snippets are derived from user-controlled task content. The suggested change prevents XSS, though it would also remove HTML-based highlighting.

High

@SerhiiYakovenko

Copy link
Copy Markdown
Owner Author

Closed again after the history rewrite; both commits now attributed correctly. Superseded by the fresh draft staged for the O'Reilly live course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant