Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ repos:
pass_filenames: false
always_run: true

# Check for hardcoded URLs in JavaScript (must use scriptRoot)
- id: no-hardcoded-urls
name: Check for hardcoded URLs in JavaScript
entry: uv run python .pre-commit-hooks/check_hardcoded_urls.py
language: system
files: '^(templates/.*\.html|static/.*\.js)$'
pass_filenames: true

# Ensure GAM clients support both OAuth and service account auth
- id: check-gam-auth-support
name: Check GAM client supports both auth methods
Expand Down
110 changes: 110 additions & 0 deletions .pre-commit-hooks/check_hardcoded_urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
#!/usr/bin/env python3
"""
Pre-commit hook to detect hardcoded URLs in JavaScript code.

Prevents bugs where JavaScript uses absolute paths like '/auth/login' or '/api/...'
instead of 'scriptRoot + /auth/login', which breaks when deployed behind a reverse
proxy with a URL prefix (e.g., /admin).

See CLAUDE.md section "JavaScript URL Handling - MANDATORY" for the correct pattern.
"""

import re
import sys
from pathlib import Path

# Patterns that indicate hardcoded URLs (common violations)
HARDCODED_URL_PATTERNS = [
# Login/auth redirects without scriptRoot
(r"window\.location\.href\s*=\s*['\"]/(auth|tenant)/", "Login redirect should use scriptRoot variable"),
# API fetch calls without scriptRoot
(r"fetch\s*\(\s*['\"]/(api)/", "API fetch should use scriptRoot variable"),
# Direct assignment of auth/api URLs
(r"(const|let|var)\s+\w+\s*=\s*['\"]/(auth|api|tenant)/", "URL variable should use scriptRoot prefix"),
]

# Exceptions that are allowed (correct patterns)
ALLOWED_PATTERNS = [
r"scriptRoot\s*\+\s*['\"]/(auth|api|tenant)/", # Correct: scriptRoot + '/auth/login'
r"`\$\{scriptRoot\}/(auth|api|tenant)/", # Correct: `${scriptRoot}/api/...`
r"url_for\(['\"]", # Python url_for() - correct
r"//.*hardcoded", # Comment mentioning hardcoded (documentation)
r"❌.*hardcoded", # Documentation showing wrong pattern
]


def check_file(filepath: Path) -> list[tuple[int, str, str]]:
"""
Check a file for hardcoded URLs.

Returns:
List of (line_number, line_content, reason) tuples for violations found
"""
violations = []

try:
content = filepath.read_text()
lines = content.split("\n")

for line_num, line in enumerate(lines, start=1):
# Skip lines with allowed patterns
if any(re.search(pattern, line) for pattern in ALLOWED_PATTERNS):
continue

# Check for hardcoded URL patterns
for pattern, reason in HARDCODED_URL_PATTERNS:
if re.search(pattern, line):
violations.append((line_num, line.strip(), reason))
break

except Exception as e:
print(f"Error reading {filepath}: {e}", file=sys.stderr)

return violations


def main(filenames: list[str]) -> int:
"""
Check all provided files for hardcoded URLs.

Returns:
0 if no violations, 1 if violations found
"""
all_violations = []

for filename in filenames:
filepath = Path(filename)
if not filepath.exists():
continue

violations = check_file(filepath)
if violations:
all_violations.append((filepath, violations))

if all_violations:
print("❌ Found hardcoded URLs in JavaScript code!")
print("\nJavaScript URLs must use 'scriptRoot' variable to support proxy deployments.")
print("See CLAUDE.md section 'JavaScript URL Handling - MANDATORY' for the pattern.\n")

for filepath, violations in all_violations:
print(f"\n{filepath}:")
for line_num, line, reason in violations:
print(f" Line {line_num}: {reason}")
print(f" {line}")

print("\n✅ Correct pattern:")
print(" const scriptRoot = '{{ request.script_root }}' || '';")
print(" const url = scriptRoot + '/auth/login';")
print(" fetch(scriptRoot + '/api/formats/list');")

print("\n❌ Wrong pattern:")
print(" window.location.href = '/auth/login';")
print(" fetch('/api/formats/list');")

return 1

return 0


if __name__ == "__main__":
sys.exit(main(sys.argv[1:]))
68 changes: 68 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,74 @@ def create_media_buy_raw(promoted_offering: str, ...) -> CreateMediaBuyResponse:

**All 8 AdCP tools now use shared implementations - NO code duplication!**

---

### JavaScript URL Handling - MANDATORY
**🚨 CRITICAL**: All JavaScript code MUST use `request.script_root` for URL construction to support reverse proxy deployments.

**The Problem:**
The admin UI can be deployed behind a reverse proxy with a URL prefix (e.g., `/admin`) configured via nginx's `SCRIPT_NAME`. When JavaScript uses hardcoded absolute paths like `/auth/login` or `/api/formats/list`, they fail in production:
- Hardcoded `/auth/login` → Browser tries `/admin/auth/login` → 404 (wrong path)
- Hardcoded `/api/formats/list` → Browser tries `/admin/api/formats/list` → 404 (wrong path)

**The Solution: Always Use scriptRoot**

**✅ CORRECT Pattern:**
```javascript
// In <script> tag or JavaScript block
const scriptRoot = '{{ request.script_root }}' || ''; // e.g., '/admin' or ''
const tenantId = '{{ tenant_id }}';

// For login redirects
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;

// For API calls
const apiUrl = scriptRoot + '/api/formats/list?tenant_id=' + tenantId;
fetch(apiUrl, { credentials: 'same-origin' });

// For tenant-specific routes
const tenantLoginUrl = scriptRoot + `/tenant/${tenantId}/login`;
window.location.href = tenantLoginUrl;
```

**❌ WRONG - Hardcoded Absolute Paths:**
```javascript
// DO NOT DO THIS - Will break in production with /admin prefix
window.location.href = '/auth/login'; // ❌ Fails with proxy prefix
fetch('/api/formats/list'); // ❌ Fails with proxy prefix
```

**When to Apply This Pattern:**
1. **ALL authentication redirects** - Any redirect to login/logout URLs
2. **ALL API fetch calls** - Any calls to `/api/*` endpoints
3. **ALL internal navigation** - Any `window.location.href` assignments to internal routes
4. **Template includes** - Components like `inventory_picker.html` should include `scriptRoot` in their config

**Python Code (Server-Side):**
Python code should ALWAYS use `url_for()` which automatically handles the prefix:
```python
# ✅ CORRECT - Python redirects
return redirect(url_for('auth.login'))

# ❌ WRONG - Don't hardcode URLs in Python either
return redirect('/auth/login')
```

**Testing:**
- **Local dev** (no prefix): `scriptRoot = ''` → URLs like `/auth/login`, `/api/formats/list`
- **Production** (with `/admin`): `scriptRoot = '/admin'` → URLs like `/admin/auth/login`, `/admin/api/formats/list`

**Reference Implementation:**
- See `templates/components/inventory_picker.html` for component pattern
- See `templates/inventory_unified.html` for page-level pattern
- See `templates/targeting_browser.html` for tenant-specific route pattern

**Why This Matters:**
The reference implementation deploys to production with an nginx reverse proxy that adds `/admin` prefix to all admin UI routes. Without `scriptRoot`, JavaScript redirects and API calls fail with 404 errors, breaking core functionality like inventory browsing and authentication.

---

## 🚨 CRITICAL REMINDERS

### Deployment Architecture (Reference Implementation)
Expand Down
3 changes: 2 additions & 1 deletion templates/add_inventory_profile.html
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ <h3 style="margin-top: 2rem; border-bottom: 2px solid #007bff; padding-bottom: 0

async function loadFormats() {
try {
const response = await fetch('/api/formats/list?tenant_id={{ tenant_id }}');
const scriptRoot = '{{ request.script_root }}' || '';
const response = await fetch(`${scriptRoot}/api/formats/list?tenant_id={{ tenant_id }}`);
const data = await response.json();

// Check for errors
Expand Down
3 changes: 2 additions & 1 deletion templates/add_product_mock.html
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ <h3 style="margin-top: 2rem;">Creative Formats</h3>

async function loadFormats() {
try {
const response = await fetch('/api/formats/list?tenant_id={{ tenant_id }}');
const scriptRoot = '{{ request.script_root }}' || '';
const response = await fetch(`${scriptRoot}/api/formats/list?tenant_id={{ tenant_id }}`);
const data = await response.json();

// Check for errors from API
Expand Down
9 changes: 6 additions & 3 deletions templates/components/inventory_picker.html
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ <h3 id="picker-modal-title" style="margin: 0;">Select Items</h3>
adUnitFieldId: '{{ picker_config.ad_unit_field_id if picker_config else "targeted_ad_unit_ids" }}',
placementFieldId: '{{ picker_config.placement_field_id if picker_config else "targeted_placement_ids" }}',
adUnitDisplayId: '{{ picker_config.ad_unit_display_id if picker_config else "selected-ad-units" }}',
placementDisplayId: '{{ picker_config.placement_display_id if picker_config else "selected-placements" }}'
placementDisplayId: '{{ picker_config.placement_display_id if picker_config else "selected-placements" }}',
scriptRoot: '{{ request.script_root }}' // Base path for URLs (e.g., '/admin' or '')
};

// State
Expand Down Expand Up @@ -154,7 +155,8 @@ <h3 id="picker-modal-title" style="margin: 0;">Select Items</h3>
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = config.scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -214,7 +216,8 @@ <h3 id="picker-modal-title" style="margin: 0;">Select Items</h3>
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = config.scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down
3 changes: 2 additions & 1 deletion templates/components/inventory_profile_editor.html
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,8 @@ <h5 class="mb-3 mt-4">Authorized Properties *</h5>
resultsDiv.style.display = 'none';

try {
const url = `/api/formats/list?tenant_id=${config.tenantId}`;
const scriptRoot = '{{ request.script_root }}' || '';
const url = `${scriptRoot}/api/formats/list?tenant_id=${config.tenantId}`;
console.log('Loading formats from:', url, 'config:', config);

const response = await fetch(url, {
Expand Down
3 changes: 2 additions & 1 deletion templates/edit_inventory_profile.html
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ <h3 style="margin-top: 2rem; border-bottom: 2px solid #007bff; padding-bottom: 0

async function loadFormats() {
try {
const response = await fetch('/api/formats/list?tenant_id={{ tenant_id }}');
const scriptRoot = '{{ request.script_root }}' || '';
const response = await fetch(`${scriptRoot}/api/formats/list?tenant_id={{ tenant_id }}`);
const data = await response.json();

// Check for errors
Expand Down
25 changes: 17 additions & 8 deletions templates/inventory_unified.html
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ <h6>Limits (Optional)</h6>

<script>
const tenantId = '{{ tenant_id }}';
const scriptRoot = '{{ request.script_root }}'; // Base path for URLs (e.g., '/admin' or '')

// API URL endpoints - use url_for() to generate proper URLs for dev/prod
const API_URLS = {
Expand Down Expand Up @@ -427,7 +428,8 @@ <h6>Limits (Optional)</h6>
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -545,7 +547,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -576,7 +579,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -608,7 +612,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -682,7 +687,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
if (response.status === 202 || response.ok) {
Expand Down Expand Up @@ -720,7 +726,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -785,7 +792,8 @@ <h5 class="mb-0">
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down Expand Up @@ -978,7 +986,8 @@ <h6>${item.name}</h6>
.then(response => {
// Handle authentication errors (401) by redirecting to login
if (response.status === 401) {
window.location.href = '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
const loginUrl = scriptRoot + '/auth/login?redirect=' + encodeURIComponent(window.location.pathname);
window.location.href = loginUrl;
return Promise.reject(new Error('Session expired'));
}
return response.json();
Expand Down
7 changes: 5 additions & 2 deletions templates/targeting_browser.html
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ <h3>Values for Selected Key</h3>
<!-- Sync Progress Modal -->
<script>
const tenantId = '{{ tenant.tenant_id }}';
const scriptRoot = '{{ request.script_root }}'; // Base path for URLs (e.g., '/admin' or '')
let currentKeyId = null;
let targetingData = {
customKeys: [],
Expand Down Expand Up @@ -266,7 +267,8 @@ <h3>Values for Selected Key</h3>

// Check if we got redirected to login
if (response.redirected || response.status === 302) {
window.location.href = response.url || `/tenant/${tenantId}/login`;
const loginUrl = scriptRoot + `/tenant/${tenantId}/login`;
window.location.href = response.url || loginUrl;
return;
}

Expand Down Expand Up @@ -458,7 +460,8 @@ <h5>${key.display_name || key.name}</h5>

// Check if we got redirected to login
if (response.redirected || response.status === 302) {
window.location.href = response.url || `/tenant/${tenantId}/login`;
const loginUrl = scriptRoot + `/tenant/${tenantId}/login`;
window.location.href = response.url || loginUrl;
return;
}

Expand Down