Skip to content

Conversation

@bigcat88
Copy link
Contributor

Resolves #322

Completely rewrote the check_civitai_url implementation to use urlparse and parse_qs functions instead of usual string functions to parse URLs.
Additionally expanded tests to cover the new valid URL cases.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 30, 2025
@codecov
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comfy_cli/command/models/models.py 95.12% 2 Missing ⚠️
@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   50.87%   51.12%   +0.25%     
==========================================
  Files          32       32              
  Lines        3444     3468      +24     
==========================================
+ Hits         1752     1773      +21     
- Misses       1692     1695       +3     
Files with missing lines Coverage Δ
comfy_cli/command/models/models.py 40.00% <95.12%> (+6.12%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dosubot dosubot bot added the bug Something isn't working label Sep 30, 2025
@bigcat88 bigcat88 force-pushed the fix/322/CivitAI-complex-urls branch 2 times, most recently from 9b6e997 to 2d69000 Compare September 30, 2025 07:05
@bigcat88 bigcat88 merged commit 07ef636 into main Sep 30, 2025
14 checks passed
@bigcat88 bigcat88 deleted the fix/322/CivitAI-complex-urls branch September 30, 2025 07:17
@Arcitec
Copy link
Contributor

Arcitec commented Sep 30, 2025

@bigcat88 Hey man. I stumbled on this somehow.

Anyway, the reason why it "worked" before my refactor is that the code was 100% BROKEN before my refactor and ALWAYS set the CivitAI API token headers even for non-CivitAI websites.

I fixed that and made it only set the token for CivitAI... but of course I didn't rewrite the code that detects if a URL is a CivitAI URL. I think I cleaned it up a bit, but I didn't fundamentally change the URL detection.

Saw that you rewrote the URL detector now to detect all URL types with robust methods. Great work! :)

@bigcat88
Copy link
Contributor Author

I fixed that and made it only set the token for CivitAI... but of course I didn't rewrite the code that detects if a URL is a CivitAI URL. I think I cleaned it up a bit, but I didn't fundamentally change the URL detection.

We could have avoided this situation only if we had tests for that URLs at the time.

No complaints about you; your PR did everything correctly and is very important from a security POV.
As you correctly wrote, it simply uncovered another bug that was not covered by tests.

@Arcitec
Copy link
Contributor

Arcitec commented Sep 30, 2025

Yeah. :) Thank you so much for this project. Comfy-CLI is an amazingly useful project, and I appreciate your great work and quality mind. It was fun to collaborate with you and addressing your feedback on the previous PR. It became better when we settled on using custom environment names and I am glad you caught that design mistake, since it could have become a mess with time if we didn't standardize the envs. 👍 😸 I appreciate everything you do. 🙏

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

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CivitAI download with token broken

3 participants