[server] Increase token length

[ua741]

Description

And clean up logs

Tests

[claude]

PR Review

Overview

This PR increases the access token length for public collection/file links from 8 to 10 characters and removes sensitive OTT logging statements.

---

✅ Security Improvements

Token Length Increase (8 → 10 characters)

• Good: This significantly improves security by increasing the keyspace
• Analysis: Assuming shortuuid uses base57 encoding (typical for shortuuid), this changes the entropy from ~46 bits (57^8) to ~58 bits (57^10)
• Impact: This makes brute-force attacks considerably harder (increases attempts by ~3,249x)
• Files affected: Both collection_link.go:28 and file_link.go:35 use this constant

Log Removal

• Excellent: Removing OTT logging is a critical security fix
  • userauth.go:122: Removed log of email hash + OTT
  • userauth.go:235: Removed log of valid OTTs for user
• These logs could have exposed sensitive authentication tokens in production logs

---

🔍 Code Quality

Strengths:

• Clean, focused change with clear purpose
• Proper use of constants (no magic numbers)
• Consistent application across both collection and file links

Considerations:

1. Migration Impact: Existing 8-character tokens will continue to work, but new tokens are 10 characters

   • This is fine as old tokens remain valid until disabled
   • No breaking changes for existing shared links

2. Token Uniqueness: The code uses shortuuid.New()[0:AccessTokenLength]

   • Truncating shortuuid reduces entropy from the full UUID
   • At 10 characters with base57, collision probability is still very low for realistic scales
   • Consider: For even higher security in the future, you might want to verify token uniqueness in the database before insertion

---

📝 Suggestions

Minor:

1. Consider adding a comment explaining the security rationale for the constant value

2. The removed hardcoded OTT log at line 127 is still present:

   • userauth.go:127: log.Info("Added hard coded ott for " + email + " : " + ott)
   • This is in the hardcoded OTT path (test/dev feature), so less critical, but consider reviewing if this should also be removed or at least mask the OTT value

---

⚠️ Testing Recommendations

While the PR description mentions tests, consider verifying:

1. Creating new collection/file links generates 10-character tokens
2. Existing 8-character tokens continue to work
3. Token generation has sufficient randomness (no collisions in realistic scenarios)
4. All related functionality (password protection, abuse reports, etc.) works with the new token length

---

Summary

Verdict: ✅ Approve with minor suggestions

This is a good security improvement. The token length increase provides meaningful security benefits, and removing OTT logging fixes a potential information disclosure issue. The changes are minimal, focused, and unlikely to cause issues.

Security Score: 9/10 - Solid improvement
Code Quality: 8/10 - Clean and focused

[claude]

Pull Request Review

Summary

This PR increases the access token length for public collection and file links from 8 to 10 characters, and cleans up verbose logging in the OTT verification flow. The changes improve security while maintaining backward compatibility.

---

✅ Positive Aspects

1. Security improvement: Increasing token length from 8 to 10 characters significantly increases the entropy:

   • 8 chars: ~47.6 bits of entropy (62^8 possibilities)
   • 10 chars: ~59.5 bits of entropy (62^10 possibilities)
   • This makes brute-force attacks ~3,844x harder

2. Good logging hygiene: Removing the debug logs in userauth.go that printed email hashes and OTT values is a security best practice. These could have leaked sensitive information in production logs.

3. Improved documentation: The update to users.md adds clarity about where to find verification codes and the exact log format to search for, which improves the self-hosting experience.

4. Consistent application: The constant is properly shared between collection_link.go and file_link.go, ensuring both use the same token length.

---

🔍 Observations & Suggestions

1. Backward Compatibility (Medium Priority)

The change increases token length, but existing 8-character tokens in the database will still work (they're stored and validated directly). However:

• Question: Are there any URL length constraints or client-side hardcoded length validations that might break with 10-char tokens?
• Recommendation: Verify that client applications don't have hardcoded token length validations.

2. Potential URL Collision (Low Risk)

Using shortuuid.New()[0:AccessTokenLength] truncates a UUID to 10 characters:

• At high volume, this could theoretically cause collisions (birthday paradox)
• The database likely has a uniqueness constraint that would catch this
• Question: Is there retry logic if token generation collides with an existing token?

Looking at line 64-83 in collection_link.go, there's error handling for ErrActiveLinkAlreadyExists, but this appears to be for collection-level uniqueness, not token collisions.

Recommendation: Consider adding a retry loop for token generation if a collision occurs during insertion.

3. Testing Considerations

• Manual testing needed: Verify that existing 8-char tokens continue to work
• Test token generation: Ensure 10-char tokens are properly generated and validated
• Test log changes: Confirm verification codes still appear correctly in logs when SMTP is not configured

4. Documentation Clarity (Minor)

The documentation update is good, but you might consider adding a concrete example of how to grep for the pattern in Docker logs.

---

🛡️ Security Assessment

No security concerns identified. The changes enhance security:

• ✅ Increased token entropy reduces brute-force risk
• ✅ Removed potentially sensitive log outputs
• ✅ No new authentication/authorization bypasses introduced

---

📊 Code Quality

Good code quality overall:

• ✅ Minimal, focused changes
• ✅ Proper use of constants
• ✅ Clean removal of debug code
• ⚠️ Minor: Consider adding a comment explaining why 10 was chosen (security considerations)

---

✅ Recommendation

APPROVE with minor suggestions. This is a solid security improvement with good cleanup. The suggestions above are optional enhancements, not blockers.

Before merging:

1. Verify no client-side length validations exist
2. Test that existing 8-char tokens still work
3. Consider documenting the security rationale in code comments

---

Review generated with assistance from Claude Code

[claude]

PR Review

Thank you for this PR! Overall, the changes look good - they improve security and documentation. Here are my findings:

✅ Positive Changes

1. Security Enhancement: Increasing AccessTokenLength from 8 to 10 characters significantly improves security:

   • 8-char tokens: ~57^8 ≈ 1.1×10^14 combinations (~47 bits entropy)
   • 10-char tokens: ~57^10 ≈ 3.9×10^17 combinations (~58 bits entropy)
   • This is a ~3,200x increase in search space, making brute-force attacks substantially harder

2. Improved Documentation: The updates to docs/docs/self-hosting/administration/users.md are excellent:

   • Added clear instructions on where to find verification codes in logs
   • Included the exact log pattern to search for
   • Better formatting and structure

3. Good Security Practice: Removing the debug logging in userauth.go that exposed OTT codes and email hashes (lines 122, 235)

🔍 Considerations

1. Backward Compatibility: The change affects both collection links (collection_link.go:28) and file links (file_link.go:35):

   • Existing 8-character tokens will continue to work (stored in DB)
   • New tokens will be 10 characters
   • URLs will change length, but this should not break existing shared links
   • ✅ This appears safe

2. Missing Information:

   • The PR description mentions "And clean up logs" but does not explain why the token length is being increased
   • Consider adding context: Was this due to a security audit? User feedback? Best practices alignment?

3. Token Generation Method: The code uses shortuuid.New()[0:AccessTokenLength]:

   • shortuuid uses base57 encoding (good - avoids ambiguous characters)
   • Slicing a UUID is cryptographically sound
   • ✅ Implementation is secure

📝 Minor Suggestions

1. Consider adding a comment in collection_link.go explaining the security rationale:

   const (
       // AccessTokenLength defines the length of public access tokens.
       // 10 characters from shortuuid provides ~58 bits of entropy.
       AccessTokenLength = 10

2. Documentation formatting: In the docs change, consider adding a note that verification codes are only logged when SMTP is not configured (you already did this, which is great!)

🎯 Summary

Code Quality: ✅ Excellent
Security: ✅ Improved significantly
Documentation: ✅ Much clearer
Backward Compatibility: ✅ No breaking changes expected

Recommendation: Approve - This is a solid security improvement with good documentation updates.

[claude]

Code Review

I've reviewed PR #7423 which increases the access token length from 8 to 10 characters and cleans up logging. Here's my feedback:

✅ Positive Changes

Security Improvement (server/pkg/controller/public/collection_link.go:28)

• Increasing AccessTokenLength from 8 to 10 is a good security enhancement
• This affects both collection links and file links (both use the same constant)
• Improves token entropy: ~35.5 bits (8 chars) → ~44.4 bits (10 chars) using shortuuid's base57 alphabet
• Reduces brute-force attack surface significantly

Log Cleanup (server/pkg/controller/user/userauth.go)

• Removed sensitive logging of OTTs and email hashes (lines 122, 235) ✅
• This is a good security practice - verification codes should not be logged in production

Documentation Update (docs/docs/self-hosting/administration/users.md)

• Clearer instructions for finding verification codes in logs when SMTP is not configured
• Provides specific log pattern to search for
• Better user experience for self-hosters

🔍 Considerations & Questions

1. Backward Compatibility

• Existing 8-character tokens in the database will continue to work (no breaking change)
• New tokens will be 10 characters
• This is handled correctly by the code

2. Security Analysis

• shortuuid.New() generates 22-character UUIDs using base57 alphabet
• Taking first 10 characters provides ~44.4 bits of entropy (vs ~35.5 for 8 chars)
• While better, consider if this meets your security requirements for publicly accessible links
• Industry standard recommendation is often 128+ bits for security-critical tokens

3. Missing Context

• Was there a specific security incident or vulnerability that prompted this change?
• Have you analyzed if 10 characters provides sufficient protection for your threat model?
• Consider documenting the rationale in code comments

4. File Link Consistency

• Good that this affects both collection_link.go and file_link.go since they share the constant
• Both controllers create public access tokens with the same mechanism

📝 Suggestions

Optional Enhancements:

1. Consider adding a constant comment explaining the security rationale:

   // AccessTokenLength defines the length of public access tokens
   // 10 chars from base57 provides ~44.4 bits of entropy
   const AccessTokenLength = 10

2. The hardcoded OTT logging on line 127 still exists:

   log.Info("Added hard coded ott for " + email + " : " + ott)

   Consider removing the OTT value from this log as well for consistency

3. Consider if URL collision checking is needed given the shortened token length (though collision probability is still extremely low)

🎯 Verdict

Approved with minor suggestions. The changes improve security posture and reduce sensitive data exposure in logs. The token length increase is a sensible enhancement, though you may want to evaluate if even longer tokens are warranted based on your security requirements.

The documentation update is helpful for self-hosters. Overall, this is a clean, focused PR that makes the system more secure.