v0.6.60 - #295
Open
roncodes wants to merge 26 commits into
Open
Conversation
The contract job pinned the reusable workflow to @dev-v0.7.53, a pre-release branch. That branch is now merged (fleetbase/fleetbase#575) and v0.7.53 is tagged, with fleetbase/fleetbase-api:v0.7.53 published to Docker Hub. - pins the reusable workflow to @v0.7.53 instead of the dev branch, so runs are reproducible rather than tracking a branch that can move or be deleted - passes fleetbase-ref: v0.7.53 explicitly. The reusable workflow still defaults that input to dev-v0.7.53, so without this the job would boot the stack from the pre-release branch while testing against the released image. Passing it makes the booted source and the published image the same commit. Bump both refs together at each release. Contract runs on this repo were previously failing before they reached Postman — the installer step died building the console image, because console/package.json and console/pnpm-lock.yaml were briefly out of sync on the release branch and console/Dockerfile installs with --frozen-lockfile. That is fixed in v0.7.53. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fleetbase/fleetbase#578 changed the reusable workflow to default fleetbase-ref to main and to test against fleetbase/fleetbase-api:latest, so there is no longer a per-release ref to bump here. Drops the explicit fleetbase-ref and moves the workflow reference from @v0.7.53 to @main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
POST /v1/customers accepts an optional `place` object, and the documented payload in the Postman collection sends one. Creating it always failed: SQLSTATE[HY000]: General error: 1364 Field 'location' doesn't have a default value `places.location` is a NOT NULL POINT column with no database default, and the attribute allow-list in resolveCustomerPlace is address-only -- a caller cannot supply coordinates through this surface -- so Place::create was always called without one. Every documented signup that included a place returned a 500. Default it to Point(0, 0), the same placeholder the geocoding helpers already fall back to when an address cannot be resolved (Place::getGoogleAddressArray, Place::findExistingSharedPlace). The default sits in the base array of the array_merge, and `location` is not in the allow-list, so a caller can never override it. Verified against a live stack: the documented Create a Customer payload now returns 201 with the address resolved, and Place::create without a location still fails with 1364 -- confirming the default is what fixes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
POST /v1/drivers/verify-code minted a driver token with no verification code
at all, on a default install.
$verificationCode = VerificationCode::where([...])->exists();
if (!$verificationCode && $code !== config('fleetops.navigator.bypass_verification_code')) {
return response()->apiError('Invalid verification code!');
}
`$code` is unvalidated, so omitting it from the body makes it null. The config
resolves to env('SMS_AUTH_BYPASS_CODE', env('NAVIGATOR_BYPASS_VERIFICATION_CODE')),
which is null when neither is set -- the default. `null !== null` is false, so
the whole condition is false and the guard never fired. The user lookup is not
company-scoped, so any caller holding a valid org API credential could mint a
token for any driver in the install.
Verified against a live stack before the fix: a POST carrying only
{"identity": "<driver phone>"} returned HTTP 200 with a Sanctum token.
Replace the comparison with a shared guard requiring all three conditions the
console equivalent already uses (AuthController::authenticateWithVerificationCode):
the bypass code must be non-empty, the app must not be in production, and the
comparison is constant-time. This closes three defects at once -- the
null-equals-null bypass, the missing production gate, and the non-constant-time
compare.
The guard lives on Api\v1\DriverController and is called from Internal\v1 so the
two verify-code paths cannot drift. `!== null && !== ''` is used rather than
`!empty()` so a configured bypass code of "0" still works.
Truth table over every relevant combination confirms the only behaviour changes
are the two bypassable cases becoming rejections; configured, matching,
non-production use is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signing up a customer locally means waiting on a real email or paying for a real SMS, because the three code-checking endpoints (POST /v1/customers, /customers/verify-code, /customers/reset-password) match against real VerificationCode rows and there is no way to short-circuit them. Navigator already has fleetops.navigator.bypass_verification_code for exactly this need on the driver side; this is the customer equivalent. Adds fleetops.customers.verification_bypass_code, read from FLEETOPS_CUSTOMER_VERIFICATION_BYPASS_CODE, guarded by the same three conditions the console uses in AuthController::authenticateWithVerificationCode: a code must be configured, the app must not be in production, and the comparison is constant-time. With the variable unset -- the default -- the bypass cannot fire, and config/app.php resolves `env` to production when neither APP_ENV nor ENVIRONMENT is set, so it fails safe. Deliberately a distinct env var, not SMS_AUTH_BYPASS_CODE: that one already gates operator console login and driver login, and sharing it would make a single leaked value unlock three privilege tiers. The guard is intentionally NOT folded into verificationCodeExists() / findVerificationCode(). Those are test seams the controller contract tests override, so a policy living inside them would be stubbed away precisely where it needs asserting. CreateCustomerRequest's `code` rule is relaxed from `exists:verification_codes,code` to `required|string`. This is not a weakening: the controller matches code + for + meta->identity, whereas the `exists` rule accepts any live code issued for any purpose to any user. The rule is also already unenforced on the proxy path, since verifyCode() with for=fleetops_create_customer calls create(CreateCustomerRequest::createFrom()), which never runs validateResolved(). Left in place it would block the bypass before the controller ever sees the request. Tests cover: inert when unset and when empty; accepted for all three endpoints when configured and matching; a non-matching code still rejected while the bypass is live; rejected in production even when configured; and resetPassword surviving the null VerificationCode on the bypass path while still revoking sessions. 10/10 in ApiCustomerControllerContractsTest, 21/21 in RequestContractsTest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GET /v1/onboard/driver-onboard-settings/{companyId} passed the result of
findCompanyByPublicId() straight into driverOnboardSetting() without checking
it. The lookup is declared ?Company and returns null for any public id that
does not resolve, so null->uuid yielded null, which then hit the string type
declaration on driverOnboardSetting() and threw:
TypeError: driverOnboardSetting(): Argument #1 ($companyUuid) must be of
type string, null given
That escaped as an unhandled exception and rendered a ~1.2 MB HTML stack
trace with a 500. Reproduced against a live stack with both a bogus public id
and the literal "{{organization_id}}" the Postman collection was sending; a
valid public id was unaffected.
An unknown organization is a client error, so guard the null and return 404
through a new errorResponse() seam, matching the protected-seam style the rest
of this controller already uses.
The existing probe overrode findCompanyByPublicId() to always return a
hydrated Company, so the null branch was unreachable in tests. It can now be
made to miss, and a regression test pins the 404 and its error envelope.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ansaction id POST /v1/parts and POST /v1/fuel-transactions returned HTTP 500 on a well-formed body. Both are the same defect: a UNIQUE index exists, the FormRequest has no matching `unique:` rule, and the resulting UniqueConstraintViolationException escapes uncaught. Public v1 controllers extend the bare Controller with no QueryException catch, so it reaches the client as a 500 with no field attribution. (Internal /int/v1 controllers get HasApiControllerBehavior, which catches it — hence v1-only.) Two aggravating factors surfaced while reproducing: Neither unique index was soft-delete aware, so a deleted row kept occupying the key. Verified end to end: DELETE a part, then POST the same SKU, and the API returns 500 — permanently, since there is no restore or force-delete route. A validation rule alone would not fix this; it would pass validation and still fail at the driver. Both indexes are now soft-delete aware via a STORED generated column that is NULL for deleted rows, following the pattern in alrashed 2026_05_25_000001. fuel_provider_transactions was also keyed globally on (provider, provider_transaction_id) with no company_uuid, and FuelProviderService::ingestTransaction() keyed its updateOrCreate on the same pair. Neither model registers CompanyScope, and CompanyScope is inert under CLI where the sync runs, so one company's sync could find and overwrite another company's transaction, reassigning its company_uuid. The index and the ingest key both gain company_uuid. Both new indexes are strictly more permissive than the ones they replace, so no existing row can violate them and no backfill is required. The parts replacement is created before the old index is dropped: the old one was the only index covering company_uuid and parts_company_uuid_foreign depends on it, so dropping first fails with errno 150. Verified against a live MySQL 8.0.41 stack: migration applies (and is re-runnable), both indexes and generated columns land as intended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nches All three workflows filtered pull_request on `branches: [main]`, so a PR targeting a dev-v* release branch triggered no checks at all. Release work lands on the release branch first and only reaches main via the release PR, so without this every contributing PR merges unverified and the first real signal arrives after the fact, on the release PR itself. Add dev-v* to the push and pull_request filters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 9766 9803 +37
===========================================
Files 521 523 +2
Lines 37762 37861 +99
===========================================
+ Hits 37762 37861 +99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CustomerEndpointTest matches against the literal source text of CreateCustomerRequest, so relaxing the `code` rule from `required|exists:verification_codes,code` to `required|string` failed the suite even though the behaviour under test was unchanged: Test Failed (CustomerEndpointTest::__pest_evaluable_FormRequest_validators _are_present_and_authorize_via_api_credential) Update the expected string and note the coupling, so the next rule change tells the reader why this file has to move with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…vironment()
verificationBypassMatches() asks app()->environment('production') — correctly,
since a bypass code left set in a deployed .env must not work against a live
fleet. The harness binds a bare Illuminate\Container\Container, which has no
such method, so three tests errored:
Call to undefined method Illuminate\Container\Container::environment()
DriverControllerAuthFlowsTest::code_verification_handles_unknown_users_
invalid_codes_bypass_and_success
DriverControllerAuthFlowsTest::code_verification_reports_token_issuance_failures
DriverControllerAuthFlowsTest::token_persistence_failures_are_reported_to_sentry
Swap in a container subclass exposing environment() and hasDebugModeEnabled(),
copying the existing bindings across by reflection. This mirrors
fleetopsCustomerHelperContainer() in CustomerControllerHelperSeamsTest rather
than inventing a second approach.
The swap has to run before the app()->instance() calls in the boot function, or
those bindings land on the container being superseded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 100% line-coverage gate failed at 99.96%: 84.21% 48/57 server/src/Http/Requests/CreateFuelTransactionRequest.php 91.43% 32/35 server/src/Http/Requests/CreatePartRequest.php A coverage slice put the gap on exactly three things — both messages() overrides, the fuel request's ignore-self clause, and resolveProvider()'s stored-provider fallback. RequestContractsTest gains the messages() assertions and the fuel ignore-self case. It supplies `provider` in the body so resolveProvider() short-circuits on the input, keeping that file free of a database. The fallback itself does query, so it is covered in FuelProviderTransactionControllerContractsTest, whose fixture already has a fuel_provider_transactions table and a seeded petroapp row. Asserting it matters: a null scope would compare the transaction id against the wrong set of rows, which is the failure the unique rule exists to prevent. Verified by slice — CreatePartRequest is now fully covered, and CreateFuelTransactionRequest retains only authorize() (lines 11 and 13), which is exercised by another file and is outside the 9 lines CI counted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l harness
The API-side fix left the internal verify-code path failing the same way, since
Internal\v1\DriverController now defers to the shared
DriverController::verificationBypassMatches() and therefore also reaches
app()->environment('production'):
Call to undefined method Illuminate\Container\Container::environment()
DriverControllerContractsTest::internal_driver_controller_verify_code_covers_
missing_user_invalid_code_and_missing_driver_branches
DriverControllerContractsTest::internal_driver_controller_verify_code_returns_
driver_resource_and_handles_token_errors
Hook the swap into FleetOpsInternalDriverAuthControllerProbe::resetProbe(),
which both tests already call, rather than repeating it per test. The helper
short-circuits when the container is already swapped, so calling it per test
costs nothing.
Verified locally: both this file and Api/DriverControllerAuthFlowsTest pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ci: fix the Postman contract and track the latest release automatically
codecov failed on this branch alone. The 404 assertion added in
SmallControllerContractsTest goes through FleetOpsPublicNavigatorControllerProbe,
which overrides errorResponse() to keep the fixture small — so the real
one-liner never executed and the new seam was the only uncovered code in the
file.
The other seams in this controller are covered by the SQLite-backed navigator
test in GeofenceDwellAndBulkNotifyTest, which drives the real controller, so
the missing-company case belongs there too.
Only the status is asserted. The harness `response()` shim envelopes errors as
{"error": ...} while the core-api macro it stands in for produces
{"errors": [...]}; the status code is the part that holds in both, and the
payload shape is already pinned by the probe-based test.
Verified by slice: NavigatorController has no uncovered lines.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s-404 fix(api): answer 404 instead of 500 for an unknown onboard organization
…-parts-fuel-transactions fix(api): return 422 instead of 500 on duplicate part SKU and fuel transaction id
fix(drivers): close verify-code authentication bypass
…fault fix(customers): default a Place location so signup with a place works
…ass-code feat(customers): add a non-production verification-code bypass
With build-from-source: false the stack boots the published API image, and this package is a composer dependency baked into it — so a PR here booted the released version and ran the collections against that. Its own API changes were never exercised; the check was green on code that was not under review. overlay-package makes the reusable workflow check this repository out at the commit under test and swap it into the running container, dumping the autoloader (the image is built with --optimize-autoloader, so a frozen classmap would otherwise hide classes added or moved on the branch), clearing caches and running migrations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ci(postman): test this branch's API code, and unpin the contract workflow
The bypass code was compared against the submitted code alone, so anyone who learned it could authenticate as ANY driver or customer. The mitigation in place — refusing the bypass outside production — closed that hole but broke the thing the bypass exists for: an app store reviewer tests a release build against production, which is exactly where it was refused. Scopes the bypass to explicitly designated accounts instead. A code is only honoured for an identity listed in the new review_accounts config, so it keeps working for reviewers in production while a leaked code authenticates nobody. NAVIGATOR_REVIEW_ACCOUNTS=+15555550100,apple-review@example.com FLEETOPS_CUSTOMER_REVIEW_ACCOUNTS=... Both the code and the allowlist are required and neither has a default, so an unconfigured install has no bypass at all. Comparison stays constant-time, and each accepted bypass is logged with its identity so use is auditable. Extracted into a trait rather than shared between the two controllers directly: they are siblings, so a protected static on one is not callable from the other — a fatal that php -l does not catch. The reset-password path compares the normalised identity ($needle), matching how the other verify paths normalise in place, so an allowlisted phone number is compared in the same form it is stored. Verified against the real trait: a listed account with the right code passes; an unlisted account with the right code is refused; wrong code, absent code, empty allowlist and null identity all refuse; matching is case- and whitespace- insensitive; and a configured code of "0" is honoured rather than dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suites encoded the policy the previous commit replaced — that a configured bypass works outside production and is refused inside it. With the bypass now scoped to designated accounts, those assertions no longer describe the code. - "accepts a configured verification bypass outside production" becomes "accepts a configured verification bypass for a listed review account", and deliberately runs in production. That is the behavioural change: a reviewer tests a release build against production, so refusing it there made review impossible. Safety comes from the allowlist now, not the environment. - "refuses the verification bypass in production" becomes "refuses the verification bypass for an identity that is not listed", covering both an allowlist naming someone else and an empty one. This is the property that matters: holding the code is no longer sufficient. The driver suites configured only a bypass code and expected it to authenticate, so each now also lists the identity under test. Both the raw and phone-normalised forms are listed for the internal suite, since verifyCode passes a non-email identity through static::phone() before the check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
POST /v1/sensors failed outright:
SQLSTATE[HY000]: General error: 1364 Field 'last_position' doesn't have a
default value
Migration 2025_10_27_171322 made last_position NOT NULL on devices AND sensors
so both could carry a spatial index, but only Device got a creating hook to
default it. A position is not something a caller has when registering a sensor,
so Sensor now defaults to POINT(0,0) exactly as Device does.
Found by the Postman contract run, which had never once managed to create a
sensor — the failure cascaded into Retrieve, Update and Delete.
Adds SensorLastPositionDefaultTest mirroring the existing Device coverage: a
sensor created without a position lands on the null island, and one created
with a position keeps it.
Also fixes a trailing-comma lint violation in ResolvesReviewAccountBypass,
introduced with that trait earlier and failing `composer test:lint`.
NOT VERIFIED LOCALLY: the pest harness cannot bootstrap on this host or in the
container — server_vendor/bin/pest dies with "Class Symfony\Component\Console\
Input\ArgvInput not found" because Pest hardcodes ../../../vendor/autoload.php.
The pre-existing DeviceLastPositionDefaultTest fails identically, so this is
environmental rather than something these changes introduce. Lint is clean and
CI runs the suite with the 100% coverage gate; watching that result.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Release branch for fleetops 0.6.60, cut from
mainwithflb version-bump(patch). The bump updatescomposer.json,extension.jsonandpackage.json.Also carries a CI fix:
ember.yml,server.ymlandpostman.ymlall filteredpull_requestonbranches: [main], so any PR targeting adev-v*release branch triggered no checks at all. Release work lands here first and only reachesmainvia this PR, so without it every contributing PR would merge unverified.dev-v*added to thepushandpull_requestfilters.Contents
Merged in ascending order.
d0be554956d5c5bf983668d2728a93db6a43d21e6092b24eWhat lands here
GET /v1/onboard/driver-onboard-settings/{companyId}returned a ~1.2 MB HTML stack trace for any organization public id that did not resolve. The nullable company lookup was never guarded, sonull->uuidhit astringtype declaration and threw aTypeError. Now a 404.UniqueConstraintViolationExceptions, i.e. HTTP 500 with no field attribution. Now validated up front, scoped per company (and per provider for fuel), soft-delete aware, ignoring the record itself on update.null, so the old$code !== config(...)comparison wasnull !== null— false — and the guard never fired: any caller with a valid org API credential could mint a driver token for any driver without a code at all. Now requires a configured code, refuses to work in production, and compares in constant time.#289 is deliberately excluded — it is the one PR in this range opened from an external fork (
janni1288), and is left untouched. For whoever picks it up: itsbuildfailure isERR_PNPM_OUTDATED_LOCKFILE—leaflet@^1.9.4was added topackage.jsonwithout regeneratingpnpm-lock.yaml.CI repairs made to the contributing PRs
Four of the six arrived with a red
build. All were fixed and green before merging.NavigatorController::errorResponse()was exercised only through a probe that overrides it, so the real seam never executed and codecov failedCreateFuelTransactionRequest.php48/57,CreatePartRequest.php32/35messages()overrides, the fuel ignore-self clause, andresolveProvider()'s stored-provider fallbackCall to undefined method Illuminate\Container\Container::environment()—verificationBypassMatches()asksapp()->environment('production')resetProbe()CustomerEndpointTestasserts the literal source text ofCreateCustomerRequest, which the PR editsTwo notes for future work. #292 needed two passes: repairing the API harness surfaced the identical error in the Internal one, since both controllers now route through the same bypass helper. And #290 is the classic one-line-seam trap — a behaviour test overrides the seam to keep the fixture small, so the real body never runs and the 100% gate quietly loses a line.
🤖 Generated with Claude Code