Skip to content

feat(ocpp): merge master onto OCPP 1.6J + 2.0.1 for review - #979

Merged
frahlg merged 26 commits into
masterfrom
cursor/ocpp-review-ready-7a7a
Aug 29, 2026
Merged

feat(ocpp): merge master onto OCPP 1.6J + 2.0.1 for review#979
frahlg merged 26 commits into
masterfrom
cursor/ocpp-review-ready-7a7a

Conversation

@frahlg

@frahlg frahlg commented Aug 25, 2026

Copy link
Copy Markdown
Member

Accepted text proposal

Issue or Discussion: #732 and discussion #747.

Maintainer comment that accepted this scope: “vore ju bra med OCPP … låt oss göra dessa parallellt”.

What changed

This is #732 (HuggeK:worktree-ocpp-restore) with current master merged in so the draft can be reviewed without the 10-commit drift.

A rebase was attempted and aborted: the first commits on #732 are the units work already squash-merged as #962, and replaying them conflicts everywhere. Same merge strategy Fredrik used on 19 August.

#732 stays open. This branch does not rewrite HuggeK's history. Review the OCPP work here or on #732; do not close #732 without reading its description.

OCPP behaviour is unchanged from #732:

  • Central system for 1.6J (port 8887) and 2.0.1 (own port)
  • Chargers dial in; no Lua driver
  • Control via SetChargingProfile, never RemoteStopTransaction (Charge Amps)
  • Pending quarantine for unadopted charge points
  • Settings → Chargers panel, vehicle profiles, capability probe

Why

Charge Amps (Halo / Aura / Dawn / Luna) is local-first over OCPP 1.6J and needs no cloud driver. Easee and Zaptec can also speak OCPP after a one-time portal step. That is the Nordic coverage that a Charge Amps cloud driver would not give.

Boundaries and safety

Unchanged from #732: shared basic auth, listener on 0.0.0.0 without TLS, pending quarantine, no per-charger credentials, no /api/devices row. Hardware acceptance is still simulator + field reports.

Verification

After the merge:

go test ./internal/ocpp ./internal/config ./internal/loadpoint ./internal/api -count=1
node --test web/settings/tabs/loadpoints.test.mjs web/setup.test.mjs

All passed (ocpp, config, loadpoint, api 30s, web 41/41).

Checklist

  • The diff implements one accepted scope and does not add follow-on work.
  • I checked open pull requests that touch the same files. This is feat(ocpp): OCPP 1.6J + 2.0.1 support — chargers connect with no driver #732 plus master.
  • Tests cover the changed behaviour and its failure path.
  • A human reviewed every changed web/UI view in a browser, or no UI changed.
  • A Changeset is included, or the change is exempt.
  • Every commit has a DCO sign-off.
Open in Web Open in Cursor 

frahlg and others added 24 commits August 19, 2026 13:14
PV arrays use rated_w. SoC, planner bounds, and related settings are
0-1. kWp and 0-100 exist only at UI, HA, appproto, calendar titles, and
the forecast.solar URL. Config load still folds the old keys. A units
package plus consistency tests reject kWp and soc_pct leaking back into
core structs.
Core stored EV SoC as 0-100 while the battery planner already used 0-1.
That mix, not a missing clamp, is what let percent-scale values leak
into watt math. Loadpoint state, schedules, boost leases, vehicle
telemetry, calendar intents, V2X envelopes, and shadow SoC are now
fractions. 0-100 remains only at UI, HA, appproto, calendar titles,
Tesla verify, and optimizer protocol v1. The units harness fails if
_pct names or kWp return to those core structs.

Nameplate ceilings stay as physics gates. They are not the unit fix.
Slot directives, live PV surplus caps, and the operator absorb cap
already stored 0-1. The Pct suffix made them look like 0-100. Rename
the fields, convert golden dump keys, and fail the units harness if
those names return.
Brings back go/internal/ocpp, retired as unused in #578, so EV chargers can
connect to FTW directly instead of through a vendor cloud.

The package is restored unchanged and still builds, vets and passes its own
tests against the current tree. github.com/lorenzodonini/ocpp-go resolves to
v0.19.0 at @latest — the same version that was removed, since upstream has not
cut a release since August 2025.

Nothing is wired into main.go yet, so this changes no runtime behaviour. Two
known gaps are carried over from the original and must be closed before the
server is enabled:

- Config.Bind is advisory only. ocpp-go does not expose a bind address, so the
  listener takes 0.0.0.0 regardless, and Phase 1 has no TLS.
- Handlers are read-only. Charge Amps needs SetChargingProfile-based control,
  because its RemoteStopTransaction is unreliable in the field.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
…argers

Wires the restored OCPP 1.6J Central System into the process behind a new
opt-in ocpp config section, and documents the part that is easy to miss: an
OCPP charger needs no driver at all.

OCPP is vendor-neutral, so one server in core covers every charger that speaks
it. Chargers dial FTW rather than being polled, so there is nothing to add
under drivers: — a charge point becomes a device on its first BootNotification,
keyed by the last segment of the URL it connected to.

Enabling the server requires a username and password, and config validation
rejects an enabled section without them. This is deliberate. ocpp-go builds its
listen address from the port alone, so the socket is reachable on every
interface and cannot be pinned to one; basic auth is the only gate in front of
it. That is a mitigation, not a fix, and the docs say so.

Documented in docs/ocpp.md, with pointers from the README, config.example.yaml
and writing-a-driver.md so nobody starts a Lua driver for a charger that does
not need one.

Tests cover the fail-closed credential rule and assert the shipped example
config still parses, ships disabled and validates.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
… stop

FTW can now throttle, pause and resume an OCPP charger. Command has the same
signature as drivers.Registry.Send and speaks the vocabulary every EV driver
already implements, so main.go routes by name and loadpoints never learn that
an OCPP charger is not a Lua driver.

Every command is a current limit, never a remote start or stop.
RemoteStopTransaction is unreliable on Charge Amps hardware — units acknowledge
the stop and then resume charging on their own — while a 0 A charging profile is
honoured consistently. It also leaves the transaction open, so the session meter
keeps counting across a pause instead of splitting into two sessions.

Two safety decisions worth calling out:

- Below the IEC 61851 minimum of 6 A the charger is told 0 A rather than rounded
  up. When the allocator has less headroom than that to give, rounding up draws
  current the site fuse was never asked to carry, so refusing to charge is the
  safe direction of error.
- A pause records no limit, so resuming returns to the last non-zero rate rather
  than the fallback ceiling. Caught by TestResumeRestoresLastLimit.

Also splits charger state in two. connected already meant "a connector has a
vehicle on it" and was never set by OnConnect, so it could not gate control: a
default charging profile is exactly the thing you set on an idle charger. online
now tracks the WebSocket session and is what control gates on.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
MaskSecrets covered every other credential section but not the new ocpp one, so
GET /api/config returned the password in plaintext to any UI client. That is
the one credential that must not leak: the OCPP listener is reachable on every
interface and basic auth is the only thing in front of it.

Masking alone would have traded a leak for a wipe, because the settings tab
posts the config back and the masked password returns empty.
PreserveMaskedSecrets now keeps the stored value when the incoming one is
blank, while a genuinely new password still wins.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Newer chargers now connect without a driver too. Each version listens on its
own port, because a charger picks its dialect during the WebSocket handshake
before any message is sent, and ocpp-go's ws.Server keeps a single message
handler per listener — one port cannot serve both. Set ocpp.port_v201 to enable
2.0.1; leaving it unset keeps 1.6J only.

Only the encoding differs. Both dialects share one charger map, one telemetry
path and one control path, so a 2.0.1 charger is metered, throttled and paused
exactly like a 1.6 one and dispatch cannot tell them apart. Which listener a
charger reached is recorded on connect, and control encodes the profile to
match.

2.0.1 restructures more than the names suggest: Start/StopTransaction collapse
into one TransactionEvent, transaction ids become strings, connector status
loses its charging meaning, and meter samples arrive inside transaction events
as well as alone. The new handler normalises all of it back to the same state.

OCPP 2.1 is deliberately absent. No production-grade Go implementation exists —
ocpp-go covers 1.6 and 2.0.1 only, and the Go projects claiming 2.1 are
early-stage validators and emulators rather than servers. Adding it later is one
handler and one listener; the version-neutral core is unaffected.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Records the boundary between the vendored protocol layer and FTW's own code,
so a reader knows which side of it a bug is on: transport, OCPP-J framing,
message types and schema validation are ocpp-go; handlers, telemetry mapping,
control semantics and safety clamps are ours.

Also corrects a false claim carried over from the retired package. The doc
comment said ocpp-go was "also used by SteVe" — SteVe is a Java project
(GPL-3.0), so it cannot depend on a Go library, and ocpp-go's README names no
production users at all. Replaced with facts that can be checked: MIT, v0.19.0,
no release since August 2025, and upstream's own description of its 2.0.1
support as needing more real-world testing.

The package doc was stale in other ways too — it still described a 1.6-only,
read-only server whose Phase 2 would add control, all of which has since landed.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
The brand-cleanup workflow inventories "MIT licensed" as classified copy, so
the new provenance paragraph tripped it. Same fact, different wording.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Rename the Loadpoints settings tab and dashboard section to Chargers —
the thing an operator recognises is the charger, not the internal
loadpoint binding (config keys keep their spelling).

The tab gains an OCPP panel: the exact backend URL to enter on a
charger, how to enable the server when it is off, live state for every
connected charge point, and a warning to reserve the FTW host's IP in
the router before commissioning — chargers store the URL and some
whitelist addresses, so a DHCP move silently orphans them.

Charge points seen by the OCPP server now appear in the charger-driver
dropdown, so binding an OCPP charger to the planner is a dropdown pick
instead of a YAML edit. Backed by GET /api/ocpp/chargers, which reports
the effective ports plus a per-charger view extended with online state,
dialect, vendor/model and the last accepted limit.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
A charge point that authenticates but is not named by any charger entry
(loadpoint) now connects as "pending": visible in Settings -> Chargers and
GET /api/ocpp/chargers so it can be adopted, but its telemetry never
reaches the site - no DerEV reading, no driver health, no metrics - and it
is never commanded. Without this, any device holding the shared basic-auth
secret could fabricate EV load and suppress home-battery discharge through
the dispatch clamp.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Loadpoints hot-reload on config save, so the quarantine's approved set must
follow in the same applier — otherwise adopting a pending charger would arm
the loadpoint controller immediately while telemetry stayed blocked until a
restart. Revoking an adoption now also pushes a zero DerEV reading, for the
same reason OnDisconnect does: the last power figure must not linger in a
sum the charger is no longer part of.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
… 2.0.1/15118

Note on the config field, the settings tooltip and docs/ocpp.md: a charger
entry's vehicle_capacity_wh models the one car the charger usually serves,
and a wrong value costs planning accuracy, never safety. Detecting which car
is plugged in - to switch capacity automatically - needs vehicle identity
from the protocol: OCPP 1.6's idTag names the RFID card, not the car; real
identity (MacAddress autocharge / eMAID) and NotifyEVChargingNeeds are OCPP
2.0.1 + ISO 15118 territory, listed as future work.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
…policy

A vehicles: config list (and a Vehicles section on Settings -> Chargers)
gives each car a capacity, identifiers and a charging policy: PV-surplus-only
and/or a target SoC the planner fills toward in the cheapest hours. When a
charging session identifies the car - the RFID idTag on 1.6, a MacAddress
(autocharge) or eMAID (ISO 15118) idToken on 2.0.1 - the loadpoint switches
to that car's capacity and policy for the session. Capacity reverts on
plug-out and survives config hot-reloads. A session matching no profile
changes nothing (the visitor default), and its identity shows in the
Chargers table so the operator can paste it into a profile. Quarantine
applies: pending chargers store identity for display but never fire the
profile hook.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Shortly after a charger connects, core reads SupportedFeatureProfiles (1.6)
or SmartChargingCtrlr.Available (2.0.1), stores the raw answer and derives a
tri-state verdict: smart charging, telemetry only, or not reported. Surfaced
as steerable/feature_profiles on GET /api/ocpp/chargers and as a Control
column on Settings -> Chargers, with a warning before an operator binds a
metering-only charger to a charger entry and waits for a planner that will
never move it.

Advisory, never a gate: commands are still attempted, because vendors
under-report and firmware changes the answer. What decides remains the
response to a real SetChargingProfile, handled by the actuation tracker.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Connect and boot both trigger the probe milliseconds apart, long before any
answer can arrive, so every charger was asked twice. An in-flight marker
collapses that into one request, cleared on every callback path (and on a
send that never left) so an unanswered probe still retries on the next
connect - which is how a firmware update that adds SmartCharging is noticed.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
A probe in flight when the socket dies may never get its callback, which
would leave the in-flight marker set and block every later probe for that
charger. The reconnect is exactly when we want to ask again, so OnDisconnect
clears it.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
Vehicle profiles used 0-100 target_soc_pct. Core stores fractions;
legacy YAML target_soc_pct still folds at the config door.
The branch was rebased onto si-core-units, which has since been squash-merged
into master as #962. That left the branch one commit short of the base it was
stacked on: it carried the units work but not 1eaf45d, the fold of vendor
kW/kWh at emit_metric. Two driver tests failed in CI because of it —
TestMyUplinkEmitsAllPointsWithUnits and TestNibeLocalEmitsTelemetry both read
kW/kWh where core now stores W/Wh.

Seven files conflicted, all for the same reason: the branch holds a rebased
copy of the units commits while master holds the squashed one, so identical
content arrives with no common ancestor. Each conflict is a union, resolved
toward whichever side is the superset:

  go/internal/units/units{,_test}.go        master — adds CanonicalPowerEnergy
  go/internal/config/units{,_test}.go       branch — adds the Vehicles loop
  go/internal/config/config.go              branch — OCPP + Vehicles types;
                                            master's side was only a gofmt
                                            realignment of the same fields
  go/internal/loadpoint/loadpoint.go        branch — VehicleName/vehicleName
                                            session fields
  web/diagnose.js                           master — its socValue helpers also
                                            read legacy soc_pct snapshots,
                                            which the branch's copy had lost

Nothing was dropped in either direction: the merged tree keeps the emit_metric
canonicalisation and the OCPP vehicle-profile work side by side.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
.changeset/si-core-units.md survived the merge because this branch carries a
rebased copy of the units commits while master carries the squashed one: master
added the file and then consumed it in release #956, so relative to the merge
base only this branch appeared to add it.

The entry it describes is already in CHANGELOG.md, attributed to f258a4d.
Keeping the file would print the same paragraph a second time in the next
release. Master's .changeset/ is empty; this PR's six OCPP changesets are the
only ones that should be pending.

Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
…ready-7a7a

Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
@frahlg
frahlg marked this pull request as ready for review August 25, 2026 13:22
Comment thread go/cmd/ftw/main.go
Comment thread go/internal/config/config.go
Comment thread go/cmd/ftw/main.go
"path", ocppSrv.Path(),
"note", "listener is reachable on every interface; basic auth gates the socket, and chargers no loadpoint names stay pending outside telemetry")
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCPP changes skip restart prompt

Medium Severity

The OCPP central system starts only at process boot. Hot-reload updates approved ids, but not enablement, ports, credentials, path, or heartbeat. RestartRequiredFor never lists ocpp, so Settings can report that no restart is needed after those changes while the running listener stays stale or never starts.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1d422e9. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d422e92c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread go/cmd/ftw/main.go
Comment on lines +1783 to +1785
send = func(ctx context.Context, name string, payload []byte) error {
if ocppSrv.Handler().IsOnline(name) {
return ocppSrv.Command(ctx, name, payload)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route periodic OCPP dispatch through the OCPP server

For every OCPP-backed loadpoint, the normal periodic ev_set_current path never uses this new send wrapper: sendOutcomeWithDeadline prefers the non-nil outcome sender, and main still wires that sender directly to reg.SendWithOutcome. Because an OCPP identity is deliberately absent from the Lua registry, each control tick returns driver "<id>" not found instead of calling ocppSrv.Command, so the advertised planner throttling and pausing do not work. The outcome and cycle sender paths need the same OCPP-aware routing as the plain sender.

Useful? React with 👍 / 👎.

Comment on lines +166 to +168
if req.EventType != transactions.TransactionEventEnded {
s.lastPowerW = powerW
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve power when a 2.0.1 event omits a power sample

When a valid OCPP 2.0.1 TransactionEvent contains no Power.Active.Import sample, sampledValuesV201 returns the zero value for powerW and this assignment publishes 0 W even if the EV is still charging; an event containing only an energy register has the same effect, and OnMeterValues repeats the unconditional overwrite. That undercounts live EV demand and can let the home battery discharge into a charging car. Return a hasPower flag and retain the previous reading unless an actual power sample was present.

Useful? React with 👍 / 👎.

Comment on lines +400 to +403
switch req.Status {
case core.ChargePointStatusAvailable, core.ChargePointStatusUnavailable:
s.connected = false
s.charging = false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Track connector state before publishing charger-wide telemetry

On a multi-connector charge point such as the documented Charge Amps Aura, every notification overwrites one charger-wide state without considering req.ConnectorId. For example, connector 1 can be charging while an Available notification from idle connector 2 sets the shared connected and charging flags false; meter reports likewise replace rather than aggregate per-connector power. This produces incorrect loadpoint state and can undercount concurrent EV load, so connector/EVSE state must be retained separately and aggregated for the charger-wide reading.

Useful? React with 👍 / 👎.

Comment on lines +230 to +233
h.mu.Lock()
s := h.chargersLocked(id)
changed := s.vehicleID != vehicleID
s.vehicleID = vehicleID

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reapply profiles when the same vehicle starts a new session

When the same RFID/idToken starts another session on the same charger, vehicleID still contains that identity from the prior session, so changed is false and the callback is not fired. Plug-out deliberately resets the applied vehicle capacity, which means the common case of the same car charging again silently falls back to the loadpoint default until another identity is seen or the process restarts. Callback deduplication needs to be session-aware rather than based on the last identity ever observed.

Useful? React with 👍 / 👎.

Comment thread go/cmd/ftw/main.go
Comment on lines +1347 to +1350
lpMgr.ApplyVehicleProfile(lpID, name, vehicle.CapacityWh)
lpMgr.SetSurplusOnly(lpID, vehicle.SurplusOnly)
if vehicle.TargetSoC > 0 {
lpMgr.SetTarget(lpID, vehicle.TargetSoC, time.Time{})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore vehicle-specific policy after the session

For an identified vehicle, these calls mutate the loadpoint's ordinary runtime target and surplus-only setting, but plug-out restores only the temporary capacity and vehicle name. Consequently the next visitor or unmatched car inherits the previous vehicle's target and surplus policy; SetSurplusOnly also invokes the existing persistence saver, so the profile can permanently overwrite the loadpoint override across restarts. Store and restore the base policy with the session-scoped profile rather than using the persistent setters without cleanup.

Useful? React with 👍 / 👎.

Comment thread go/cmd/ftw/main.go
Comment on lines +1003 to +1010
if ocppSrv != nil {
approved := make([]string, 0, len(newCfg.Loadpoints))
for _, lp := range newCfg.Loadpoints {
if lp.DriverName != "" {
approved = append(approved, lp.DriverName)
}
}
ocppSrv.Handler().SetApprovedIDs(approved)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restart or stop OCPP when its config changes

In the config hot-reload path, the only OCPP action is refreshing approved charger IDs. Changing enabled, ports, path, username, password, or heartbeat therefore updates the shared/API-visible config while the original listeners continue unchanged; notably, disabling OCPP leaves the externally reachable socket running and rotating a compromised password leaves the old credential valid until a process restart. Diff these fields and atomically stop/start the server, or explicitly restart-gate them instead of reporting the new config as applied.

Useful? React with 👍 / 👎.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment: Cursor Bugbot found 3 unresolved issues that need human review, so this is not approved. Assigned HuggeK and erikarenhill.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@cursor
cursor Bot requested review from Leitet and erikarenhill August 25, 2026 13:27
Comment thread go/internal/api/api.go
s.handle("GET /api/drivers/{name}/logs", Local, s.handleDriverLogs)
s.handle("GET /api/logs", Local, s.handleGlobalLogs)
s.handle("GET /api/support/dump", Local, s.handleSupportDump)
s.handle("GET /api/ocpp/chargers", Local, s.handleOCPPChargers)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Agentic Security Review
Severity: HIGH

GET /api/ocpp/chargers is registered as Local because vehicle_id is the RFID/idTag that authorizes a charge, and api_tiers_test.go treats this path as secret-bearing for the app passthrough. HTTP Authenticate does not use route tiers; GET/HEAD protection is the separate protectedReadPath list in security.go, which does not include this route.

A GET to a public/FQDN Host with RequireTokenForRemote therefore does not need a Bearer token, and lan_auth does not require the house password. Fetch-metadata checks also skip this path. The handler still returns charger ids, live power, vendor, pending identities, and vehicle_id. Sibling Local secret reads (/api/config, /api/caldav/credentials, /api/logs) are on both gates.

Impact: Unauthenticated HTTP clients can read charge-authorization identities and live charger state that the app path already treats as secret.

Fix in Cursor Fix in Web

Reviewed by Cursor Security Reviewer for commit 1d422e9. Configure here.

'<td><code>' + escHtml(c.id || "") + '</code></td>' +
'<td>' + escHtml(hw) + '</td>' +
'<td>' + escHtml(c.version || "?") + '</td>' +
'<td' + (c.feature_profiles ? ' title="' + escHtml(c.feature_profiles) + '"' : '') + '>' + steer + '</td>' +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Agentic Security Review
Severity: MEDIUM

Settings → Chargers builds this table with innerHTML. escHtml() copies through a text node, so &, <, and > are escaped but " is not. Charger-controlled feature_profiles is placed in title="..." here; OCPP charge-point ids are also placed in <option value="..."> later in the same tab.

A device that only knows the shared OCPP password can connect as pending. Capability probe still stores the raw SupportedFeatureProfiles / SmartChargingCtrlr string and Snapshot still returns it. A value such as "><img src=x onerror=...> breaks out of the attribute when an operator opens Chargers to adopt the device, running script in the FTW origin against the authenticated Settings/config API.

Impact: A quarantined charger can execute JavaScript in the operator UI and drive authenticated config changes, bypassing pending telemetry/command quarantine. LAN-scoped.

Fix in Cursor Fix in Web

Reviewed by Cursor Security Reviewer for commit 1d422e9. Configure here.

@miravoss26 miravoss26 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the OCPP central-system restore (1.6J + 2.0.1, basic auth, pending quarantine). This is #732 rebased onto current master per the PR description — same behavior, just replayed cleanly.

Correctness: matches the stated scope. Config validation rejects enabled-without-username/password, secret masking/preservation on save is tested directly (TestOCPPPasswordIsMaskedAndPreserved), and the pending-quarantine gate (unadopted charge points stay out of telemetry/dispatch) reads as designed. Tests are thorough (control, capability probe, both OCPP versions).

Security screen:

  • minor: the basic-auth check in Start() (both the 1.6J and 2.0.1 listeners) is a plain user == u && pass == p string compare, not constant-time. Worth a subtle.ConstantTimeCompare swap since this is the only gate in front of a 0.0.0.0 listener — cheap fix, and the PR's own docs are candid that "no TLS, shared secret" is already a soft boundary, so this closes one more gap in the same spot.
  • No secrets in the diff itself; password stays config/state.db-only and is correctly masked in the API response path.
  • No new external network destinations — this is a listener, not an outbound client.

Everything else (docs, changesets, boundaries write-up) is unusually thorough for a scope this size. Safe to merge from my read; the constant-time compare is a nit, not a blocker.

@HuggeK

HuggeK commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Tillbaka till plugget nu så ska sitta med detta mer nu!

Review follow-up on #979: the plain string compare was the only gate in
front of a 0.0.0.0 listener. subtle.ConstantTimeCompare on both fields,
shared by the 1.6J and 2.0.1 listeners.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C7xyXX1HYi2i1aax8EsCFx
@frahlg

frahlg commented Aug 29, 2026

Copy link
Copy Markdown
Member Author

Applied the review's constant-time-compare request: both listeners now share basicAuthCheck built on subtle.ConstantTimeCompare. ocpp package tests green.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment: Cursor Bugbot skipped on this head, so this is not approved. Human review is needed; no additional reviewers were assigned because two are already requested.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

Comment on lines +105 to +125
// OCPP 2.0.1 on its own port, when configured. Same handler and therefore
// the same charger state and telemetry — only the message encoding differs.
if cfg.PortV201 > 0 {
wsServer201 := ws.NewServer()
if cfg.Username != "" || cfg.Password != "" {
wsServer201.SetBasicAuthHandler(basicAuthCheck(cfg.Username, cfg.Password))
}
h201 := &handlerV201{Handler: h}
csms := ocpp201.NewCSMS(nil, wsServer201)
csms.SetProvisioningHandler(h201)
csms.SetAvailabilityHandler(h201)
csms.SetTransactionsHandler(h201)
csms.SetMeterHandler(h201)
csms.SetAuthorizationHandler(h201)
csms.SetNewChargingStationHandler(func(cs ocpp201.ChargingStationConnection) {
h.setVersion(cs.ID(), Version201)
h.OnConnect(cs.ID())
})
csms.SetChargingStationDisconnectedHandler(func(cs ocpp201.ChargingStationConnection) {
h.OnDisconnect(cs.ID())
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Agentic Security Review
Severity: MEDIUM

OCPP 1.6J and 2.0.1 share one charger state map keyed only by URL identity. Duplicate IDs are rejected on a single listener, but the same id can stay connected on both ports at once. The later connect overwrites dialect, online flag, and power, so charging profiles and DerEV telemetry follow the newest socket.

Impact: Anyone with the shared basic-auth secret and an adopted charge-point id can attach on the other dialect while the real charger is still connected, inject EV load, take steering, then disconnect and zero telemetry / mark the id offline even though the original WebSocket is still up. Home batteries can then discharge into a live charge.

Fix in Cursor Fix in Web

Reviewed by Cursor Security Reviewer for commit ebacfb5. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.

There are 5 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4416d46. Configure here.

if changed && approved && fn != nil {
fn(id, vehicleID, source)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeat sessions skip vehicle profiles

High Severity

noteVehicleID fires the vehicle-profile callback only when the identity string changes, but vehicleID is kept after the session ends while plug-out restores capacity. The same car charging again presents the same RFID or token, so the profile is not reapplied and SoC or planner sizing uses the loadpoint default instead of that car's capacity.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4416d46. Configure here.

case core.ChargePointStatusFaulted:
s.connected = true
s.charging = false
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unplug leaves stale EV power

High Severity

The 1.6 OnStatusNotification path clears connected and charging for Available and Unavailable but leaves lastPowerW unchanged. pushReading then republishes that power as DerEV. The 2.0.1 handler already zeros power on the same statuses.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4416d46. Configure here.


h.pushReading(id, s)
h.telSuccess(id)
return meter.NewMeterValuesResponse(), nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Energy-only samples wipe power

High Severity

sampledValuesV201 leaves powerW at 0 when a batch has no PowerActiveImport sample. OnMeterValues and TransactionEventUpdated then assign that 0 to lastPowerW (and treat charging as off). Energy-only 2.0.1 samples, which are common, look like the EV stopped drawing power.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4416d46. Configure here.

s.transactionID = h.nextTxID
s.transactionRef = req.TransactionInfo.TransactionID
s.sessionStartMeterWh = energyWh
s.sessionMeterWh = 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing start energy skews session

Medium Severity

A 2.0.1 TransactionEventStarted without an energy sample stores sessionStartMeterWh as 0. Later register readings then compute session_wh as the charger's lifetime import, not energy for this session.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4416d46. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a non-blocking comment: Cursor Bugbot skipped on this head, so this is not approved. Human review is needed; no additional reviewers were assigned because two are already requested.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

Comment thread go/cmd/ftw/main.go
Comment on lines +1316 to +1321
approved := make([]string, 0, len(cfg.Loadpoints))
for _, lp := range cfg.Loadpoints {
if lp.DriverName != "" {
approved = append(approved, lp.DriverName)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Agentic Security Review
Severity: HIGH

ApprovedIDs is every loadpoint driver_name, including Lua cloud EV drivers (for example easee), not only identities the operator adopted as OCPP chargers. A charge point that connects under that name is not pending: pushReading writes DerEV, and the loadpoint send wrapper prefers ocpp.Server.Command whenever IsOnline(name).

Enabling OCPP on a site that already has an Easee/Zaptec/CTEK loadpoint therefore auto-adopts that YAML driver name as a live OCPP identity. Anyone with the shared OCPP password who can guess that name can inject EV load into dispatch and intercept setpoints meant for the real Lua charger, which is the quarantine threat the pending gate was documented to stop.

Impact: Shared-password OCPP clients can hijack existing non-OCPP loadpoint names without an operator choosing that id in the Chargers adoption dropdown.

Fix in Cursor Fix in Web

Reviewed by Cursor Security Reviewer for commit 4416d46. Configure here.

Comment thread go/cmd/ftw/main.go
Comment on lines +1031 to +1038
if ocppSrv != nil {
approved := make([]string, 0, len(newCfg.Loadpoints))
for _, lp := range newCfg.Loadpoints {
if lp.DriverName != "" {
approved = append(approved, lp.DriverName)
}
}
ocppSrv.Handler().SetApprovedIDs(approved)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Agentic Security Review
Severity: MEDIUM

The OCPP central system is started only at process boot. Hot-reload refreshes ApprovedIDs but not enabled, ports, path, username, or password. RestartRequiredFor never lists ocpp, so Settings can report that no restart is needed after those changes. The websocket Basic Auth handler still holds the boot-time username and password. Disabling ocpp.enabled leaves the 0.0.0.0 listener running.

Impact: Rotating a leaked OCPP password in Settings writes the new secret to config.yaml but the old credential remains the only live gate until a process restart. Turning OCPP off in the UI does not close the socket.

Fix in Cursor Fix in Web

Reviewed by Cursor Security Reviewer for commit 4416d46. Configure here.

@frahlg
frahlg merged commit bf2c142 into master Aug 29, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants