Recent Activities
This page shows what are we working on.
-
1.0.85
-
1.0.84
-
head
-
Merge pull request #199 from breezefront/feature/structured-buildpage-188
chore(content-builder): move BuildPage and PlanPage to structured output (#188) 48d7ef -
Merge remote-tracking branch 'origin/master' into feature/structured-buildpage-188 13ea07
-
fix(content-builder): keep the failed payload out of the error log (#188)
The payload a failed build or plan carries is the merchant's own brief
read back, and a brief can hold copy that is not published yet. The
error log is routinely shipped off the server, so it is the wrong place
for it to accumulate.
Both skills now split the line: the error keeps provider, model, stop
reason, reason, detail and the payload's size, and the payload itself
goes to logger->debug(). That is the switch Magento already has —
Magento\Developer\Model\Logger\Handler\Debug suppresses the debug log in
production until dev/debug/debug_logging is turned on, and leaves it on
in developer mode — so no new setting is introduced for this. The byte
count stays on the error line, which is what tells support whether there
is anything to go looking for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> d759a2 -
style(content-builder): drop two comments that narrate their own diff (#188)
Comment-only: the token stream of both files is identical once comments
and whitespace are dropped (token_get_all, T_COMMENT / T_DOC_COMMENT /
T_WHITESPACE removed).
The `bands` clamp and the re-encode in conforming() each carried a
paragraph explaining why I changed the line. That reasoning belongs in
the commit that made the change, where it is reviewable, and both are
already there. The comment above the unreadable-document message stays:
it carries a fact the code does not — a document cut off inside the
"components" string produces no truncated stop reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 06fec3 -
fix(content-builder): keep a mangled payload, log it the same way twice (#188)
Two findings from the review of 8568ecc.
json_encode() answers false on invalid UTF-8, which is exactly what a
reply the model mangled contains — so the payload was dropped on the one
failure it was kept for. JSON_INVALID_UTF8_SUBSTITUTE keeps it, with
var_export() left behind for the depth and recursion cases. The
framework's Serialize\Json is not usable here: it takes no flags, so it
would escape every non-ASCII character in a logged page, and it throws,
which inside a throw expression would replace the real
StructuredResponseException with an InvalidArgumentException.
The cap and the middle cut move to AbstractSkill. PlanPage had its own
rule — 2000 bytes off the head — so a plan that broke at its end had the
end cut away, and the same payload was capped sixteen times shorter than
on a build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 2f9a7f -
feat(skills): a query-scoped skill contract, so a search skill can exist (#189) (#200)
* feat(skills): a query-scoped skill contract, so a search skill can exist (#189)
SkillInterface::execute() takes an `object $entity`. A product-discovery
skill has none: it is handed a phrase and the storefront scope, and the
entities are its result. #190 and #192 both stop here.
Adds, without touching anything that exists:
* Api\QuerySkillInterface — getCode/getLabel/execute(string $query,
array $history, array $options). Scope travels in $options, as it
already does through GenerateService, AiRequest and every entity
skill; a typed context object would have to be assembled by each of
the planned callers for no consumer that exists yet.
* Api\Data\QueryResultInterface + Model\Data\QueryResult — answer text
plus the entities it refers to. A query skill returns structure, not
a string: BuildPage shows the cost of the alternative, its caller
json_decodes the result and finds a malformed answer three frames
from where it was produced (BulkConsumer.php:348). Cost and latency
stay out — the usage ledger (#171) already records both per request.
* Model\QuerySkillPool — a second pool, not a widened SkillPool.
Every consumer of SkillPool assumes entity semantics: SkillSupport
and BulkAction\ViewData call getSupportedEntityTypes(), McpServer
gives each pooled skill a tool whose schema requires entityType and
entityId. One pool of a common parent type does not stay typed; it
becomes a parent type plus an instanceof in four places, and a query
skill nobody filtered out is published as a tool no client can call.
QuerySkillInterface deliberately does not share a parent interface with
SkillInterface. Declaring getCode() and getLabel() twice keeps this
change strictly additive, so no implementor — ours or a merchant's —
can break on upgrade; extracting the parent later is itself backwards
compatible, so the option costs nothing to defer.
The five existing skills, SkillPool, BulkConsumer and the MCP tool list
are untouched. Unit tests cover resolution of both kinds and both
cross-kind mistakes.
Checked on 2.4.7 as well as 2.4.8 (#197): ConstructorIntegrity,
TypeDuplication and ArgumentSequence against a standalone
magento/framework 103.0.7.0-patch10 and 103.0.8.0-patch1 — clean on
both, with the #197 widening re-introduced as a positive control to
prove the gate was live.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(skills): check the query pool and the result where they are built (#189)
Review of the contract itself turned up four places where it promised
more than it enforced.
QuerySkillPool refused a misregistered skill only through the return
type of get(): a \TypeError, which the LocalizedException handlers
around every pool call do not catch, and only on the path that calls
get() — getAll() has no return type to protect it and handed the wrong
object straight to a consumer. The members are now checked once at
construction, with the di.xml item's code in the message. SkillPool is
left exactly as it was; widening its behaviour is not part of #189.
QueryResult validated nothing, so the "structure beats a string"
argument stopped one level short of the resolver that reads it. An id
decoded from a model's JSON arrives as "42" as readily as 42, and a
resolver comparing a string against a loaded product finds nothing and
reports nothing. entity_type and entity_id are now normalised, a row
carrying neither is refused, and the optional fields are left untouched.
QueryResultInterface::getItems() documented its shape in PHPStan
generics, which Magento's TypeProcessor cannot read — isArrayType()
matches `[]` and `ArrayOf` only, so the shape would be taken for a class
name the day a webapi route names this interface. Now `array[]` for
Magento with the shape kept on a @phpstan-return beside it.
docs/DEVELOPMENT.md still said every capability implements SkillInterface
and registers into SkillPool, which is the sentence #190 would read
before putting a search skill in the wrong pool. Both kinds are
described now.
Unit suite 2208 tests. 2.4.7 and 2.4.8 validator runs repeated on the
amended tree, positive control still fires.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(skills): refuse non-scalar refs, drop null pool items (#189)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com> 9f570c -
fix(skills): refuse non-scalar refs, drop null pool items (#189) 215043
-
fix(skills): check the query pool and the result where they are built (#189)
Review of the contract itself turned up four places where it promised
more than it enforced.
QuerySkillPool refused a misregistered skill only through the return
type of get(): a \TypeError, which the LocalizedException handlers
around every pool call do not catch, and only on the path that calls
get() — getAll() has no return type to protect it and handed the wrong
object straight to a consumer. The members are now checked once at
construction, with the di.xml item's code in the message. SkillPool is
left exactly as it was; widening its behaviour is not part of #189.
QueryResult validated nothing, so the "structure beats a string"
argument stopped one level short of the resolver that reads it. An id
decoded from a model's JSON arrives as "42" as readily as 42, and a
resolver comparing a string against a loaded product finds nothing and
reports nothing. entity_type and entity_id are now normalised, a row
carrying neither is refused, and the optional fields are left untouched.
QueryResultInterface::getItems() documented its shape in PHPStan
generics, which Magento's TypeProcessor cannot read — isArrayType()
matches `[]` and `ArrayOf` only, so the shape would be taken for a class
name the day a webapi route names this interface. Now `array[]` for
Magento with the shape kept on a @phpstan-return beside it.
docs/DEVELOPMENT.md still said every capability implements SkillInterface
and registers into SkillPool, which is the sentence #190 would read
before putting a search skill in the wrong pool. Both kinds are
described now.
Unit suite 2208 tests. 2.4.7 and 2.4.8 validator runs repeated on the
amended tree, positive control still fires.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 1d185e -
Merge pull request #202 from breezefront/fix/openai-error-classification-201
fix(providers): OpenAI 401/429 were never classified, and a bulk 429 was never retriable (#201) 89ddd9 -
fix(providers): mark a provider outage retriable too, 529 included (#201)
Follow-up to the rate-limit work in this branch, which left 5xx alone.
A provider that was reachable and could not answer says nothing about the
request either, so it wants the same handling a 429 now gets: keep the bulk
operation for the admin's Retry button instead of recording it as dead work.
Model\Exception\ProviderUnavailableException carries the same
TemporaryStateExceptionInterface, and all three text providers and both image
generators raise it.
The statuses are a list - 500, 502, 503, 504, 529 - and not a `>= 500`. 501 is
the 5xx that will still be unimplemented tomorrow, and retrying it is the loop
the list exists to avoid. 529 is in it because Anthropic answers an overloaded
model with a status no range check written for the other four would catch.
The provider's own words travel with the new sentence rather than being
replaced by it. A 5xx from a proxy standing in for the endpoint carries the
only explanation there is, and two existing tests rely on exactly that: the
Gemini and OpenAI-image redaction tests use a 5xx to prove the API key is
masked on the way out. Both still pass untouched, which is the check that the
detail - and the redaction - survived the reclassification.
Verified by running it: the six new cases all fail against the previous commit,
the full suite is green (2205 tests), and di:compile's validators report no
findings over the module's 225 classes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> affc3b -
fix(providers): OpenAI 401/429 were never classified, and a bulk 429 was never retriable (#201)
handleException() decided what had gone wrong by looking for the status
number in the exception message. openai-php never writes it there, so both
branches were dead and every failure reached the merchant as the SDK's own
sentence - the reported one being "OpenAI error: Request rate limit has been
exceeded." The status is read off the exception now, the way Claude and
Gemini already read theirs.
It cannot be read the same way, though: openai-php exceptions have no
getResponse(), they carry a public $response, and only ErrorException adds
getStatusCode(). statusOf() names each shape instead of probing for one.
An exhausted balance is separated from a rate limit while we are here. OpenAI
answers both with 429 and only the error code tells them apart, and the two
want opposite handling now that one of them is retried.
Which is the second half. README promised that a 429 in Bulk Actions was
"automatically retried by Magento"; it was not retried at all, because every
provider failure reached AbstractConsumer as a plain LocalizedException and
was recorded as STATUS_TYPE_NOT_RETRIABLY_FAILED - so a store that tripped
its per-minute limit mid-run lost every remaining product permanently. A rate
limit now raises Model\Exception\RateLimitException, which carries Magento's
TemporaryStateExceptionInterface, and the consumer honours that interface on
its LocalizedException branch as it already did on the NoSuchEntityException
one. All three text providers and both image generators raise it.
Magento still does not re-run a retriable operation on its own, so the README
says what actually happens instead: the operation is listed in the admin's
bulk detail page and the Retry button re-queues it.
The unit tests fed handleException() `new \Exception('Error 429 Too Many
Requests')`, a message openai-php would never produce, which is how the dead
branches stayed green. They now build the SDK's real exception types and go
through chat() rather than reflecting into the private method.
Verified by running it: the five new OpenAI cases all fail against master -
including one that master mislabels a statusless timeout as a rate limit
because its text contains "429" - and the full suite is green (2198 tests).
setup:di:compile's own validators report no findings on the module, the #197
check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 51119b -
feat(skills): a query-scoped skill contract, so a search skill can exist (#189)
SkillInterface::execute() takes an `object $entity`. A product-discovery
skill has none: it is handed a phrase and the storefront scope, and the
entities are its result. #190 and #192 both stop here.
Adds, without touching anything that exists:
* Api\QuerySkillInterface — getCode/getLabel/execute(string $query,
array $history, array $options). Scope travels in $options, as it
already does through GenerateService, AiRequest and every entity
skill; a typed context object would have to be assembled by each of
the planned callers for no consumer that exists yet.
* Api\Data\QueryResultInterface + Model\Data\QueryResult — answer text
plus the entities it refers to. A query skill returns structure, not
a string: BuildPage shows the cost of the alternative, its caller
json_decodes the result and finds a malformed answer three frames
from where it was produced (BulkConsumer.php:348). Cost and latency
stay out — the usage ledger (#171) already records both per request.
* Model\QuerySkillPool — a second pool, not a widened SkillPool.
Every consumer of SkillPool assumes entity semantics: SkillSupport
and BulkAction\ViewData call getSupportedEntityTypes(), McpServer
gives each pooled skill a tool whose schema requires entityType and
entityId. One pool of a common parent type does not stay typed; it
becomes a parent type plus an instanceof in four places, and a query
skill nobody filtered out is published as a tool no client can call.
QuerySkillInterface deliberately does not share a parent interface with
SkillInterface. Declaring getCode() and getLabel() twice keeps this
change strictly additive, so no implementor — ours or a merchant's —
can break on upgrade; extracting the parent later is itself backwards
compatible, so the option costs nothing to defer.
The five existing skills, SkillPool, BulkConsumer and the MCP tool list
are untouched. Unit tests cover resolution of both kinds and both
cross-kind mistakes.
Checked on 2.4.7 as well as 2.4.8 (#197): ConstructorIntegrity,
TypeDuplication and ArgumentSequence against a standalone
magento/framework 103.0.7.0-patch10 and 103.0.8.0-patch1 — clean on
both, with the #197 widening re-introduced as a positive control to
prove the gate was live.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 013f50 -
fix(content-builder): keep the failed payload and name the output limit (#188)
Review of #199 found five things the structured-output move left behind.
The providers carry no logger, so a payload that is not put on the
exception is gone: StructuredResponseException now holds the raw reply
and BuildPage logs it, which is what master's logUnreadableReply() was
written to keep. Null on a truncation or a refusal, where the stop
reason settles it before a body exists.
A document cut off inside the "components" string is the one shape of
#143 the stop reason cannot catch — the envelope closes normally and
the provider reports an ordinary stop — so the sentence for an
unreadable document names the output limit itself.
Also: a negative "bands" is read as no count rather than as the number
it says, because `minimum` is outside the subset Schema\Validator
accepts and BandAudit would otherwise report "read as -2 band(s)"; and
a reply that missed the schema no longer borrows the document's
sentence, in the same words and punctuation on both skills.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 8568ec -
chore(content-builder): move BuildPage and PlanPage to structured output (#188)
Both skills asked for JSON in the prompt and dug the answer out of the reply
text afterwards. That cost us the one fact that separates the failures: a
document the provider cut off at the output limit and a document the model
mangled both reached the merchant as json_decode's "Syntax error" (#143), and
they want opposite answers — raise a number, or try again.
Both now call AiRequest::sendStructured() with a response schema (#187), and a
call that produced no usable answer arrives as StructuredResponseException with
a reason. Each skill turns the three reasons into three named sentences,
carrying the provider's own message — the one that names "Max Tokens" — whole.
PlanPage is fully structured: PlanSchema describes {"sections": [...]} and the
skill parses nothing at all. extractJson() and its json_decode are gone.
BuildPage gets as far as the schema subset reaches. A Content Builder node has
free-form "props" and recursive "children", and neither is expressible under
OpenAI strict mode — nor, therefore, under the intersection Schema\Validator
holds. So DocumentSchema schematises the envelope ("bands" an integer or null,
"unsupported" a list of strings, both checked on the way out) and the document
travels inside it as a JSON string, read by the new ContentBuilder\DocumentReader.
DocumentSchema's class comment sets out why, and what finishing the job would
take: a flat node list with parent references, which is a measurement to run
rather than a refactor to slip in here.
What that removes from Model/Skill: the fenced-block regex, the widest-bracket-run
candidates, the lone-backslash repair pass, the hand-written bracket scanner that
sliced a broken list element by element, and the envelope scraper. Around 630
lines of BuildPage. The partial-page salvage goes with them — a truncation is now
reported as a truncation before any document exists, instead of handing back a
prefix nobody could verify.
StopReason stays and is unchanged; it is read off the response field by
Provider\StructuredResponse, which is where it already was.
Checked: no json_decode or preg_match over a model reply is left in Model/Skill;
the two schemas pass Schema\Validator and lower onto all three dialects
(SkillSchemaTest); truncation, refusal and a reply that missed the shape each
produce a different named error in both skills' tests. 2185 unit tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> e506b3
-
-
1.8.22
-
Version 1.8.22 9893b7
-
fix(providers): StructuredResponseException cause type breaks di:compile on 2.4.7 (#197) (#198)
* fix(providers): StructuredResponseException cause type breaks di:compile on 2.4.7 (#197)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
* style(providers): wrap the cause-type comment (#197)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> 1d4842 -
style(providers): wrap the cause-type comment (#197)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> 602747 -
fix(providers): StructuredResponseException cause type breaks di:compile on 2.4.7 (#197)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> a961ae
-
-
head
-
Add Security page with Magento patch status
Lists patches reported by vendor/bin/patch-status in a UI grid and shows
the count of not applied patches in the Swissup menu badge.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> b7811a
-
-
1.14.1
-
1.4.11
-
Version 1.4.11 282682
-
fix(security): age out attachment links signed under a longer lifetime (#91) (#106)
* fix(security): age out links signed under a longer lifetime (#91)
isValid() honoured whatever expiry the signature carried, so the setting only
ever governed links signed after it changed. An admin who had the lifetime at
ten years and dropped it to one day still had ten years of links in the field,
each one good until its own expiry. Shortening the setting is the lever a shop
reaches for after a leak, and it did nothing to what had already gone out.
A signature is now also refused when its expiry is further off than the current
lifetime allows, so the setting bounds every link, not just the next one.
Lengthening it is unaffected: an expiry that is sooner than the new lifetime
stays inside the bound, so links already sent keep the life they were given.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(config): sort the link lifetime field where it sits in the file (#91)
sortOrder 27 rendered the field after the CAPTCHA notice at 25, the opposite of
its position in the XML. 22 puts it between the rate limit information and that
notice, where it reads.
validate-zero-or-greater went with it: validate-digits already rejects a minus
sign, so the pair only ever produced two messages for one mistake.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com> 0cd6b3 -
style(config): sort the link lifetime field where it sits in the file (#91)
sortOrder 27 rendered the field after the CAPTCHA notice at 25, the opposite of
its position in the XML. 22 puts it between the rate limit information and that
notice, where it reads.
validate-zero-or-greater went with it: validate-digits already rejects a minus
sign, so the pair only ever produced two messages for one mistake.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 558384 -
fix(security): age out links signed under a longer lifetime (#91)
isValid() honoured whatever expiry the signature carried, so the setting only
ever governed links signed after it changed. An admin who had the lifetime at
ten years and dropped it to one day still had ten years of links in the field,
each one good until its own expiry. Shortening the setting is the lever a shop
reaches for after a leak, and it did nothing to what had already gone out.
A signature is now also refused when its expiry is further off than the current
lifetime allows, so the setting bounds every link, not just the next one.
Lengthening it is unaffected: an expiry that is sooner than the new lifetime
stays inside the bound, so links already sent keep the life they were given.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 9d0261 -
Merge pull request #102 from swissup/fix/attachment-link-lifetime-91
fix(file): bind attachment downloads to the visitor, add an opt-in link lifetime (#91) d0a607 -
fix(file): require the attachment hash to be a string (#91)
?hash[]=x arrives as an array. It is truthy, so it was passed to isValid(),
where the (string) cast raises an "Array to string conversion" warning -
promoted to an exception in developer mode, so a request that should forward
to noroute returned a 500 instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 3f15a8 -
fix(security): read the attachment link lifetime in one scope (#91)
sign() and isValid() each read the setting with no store, so each resolved it
against whatever scope happened to be current. A link is signed wherever it is
rendered - the storefront, the backend, a cron run building a notification -
and always checked on the storefront, so the two reads could disagree.
With the field set per website and left at 0 by default, a notification signed
in the admin scope got a bare MAC with no expiry, and the storefront then
refused it through the branch meant to retire links handed out before the
setting was turned on. Every attachment link sent by e-mail broke while links
on the ticket page kept working.
The lifetime is now read at default scope and the field is a default-scope
setting, so both ends read one value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 81a149 -
Merge pull request #103 from swissup/claude/guest-ticket-token-79-d237f7
feat(security): give guest tickets an unguessable access token (#79) 339ab2 -
fix(security): take the token out of the URL once presented (#79)
The token stayed in the address for the whole visit, so a long-lived secret
was written to the access log on every request and left in the browser's
history, and getting back to the ticket after a reply depended on the Referer
header surviving - a store sending Referrer-Policy: no-referrer dropped the
visitor somewhere with neither token nor session e-mail.
The ticket view now hands a matching token to the session and redirects to the
same page without it, the way Magento's own guest order view does. Only the
value is kept, never a decision: it is matched against the ticket again on
every request, so a withdrawn token stops working at once rather than lasting
out the session.
The reply form no longer carries the token in a hidden field - the session
answers for it, and the page stops repeating the secret in its HTML.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 6545b1 -
fix(security): keep the ticket token out of customer links (#79)
TicketAccess decides a customer's ticket on the customer session alone and
returns before the token is ever looked at, so a token in that link grants
nothing. It was still mailed out, and the ticket does not stay a customer's:
the customer_id foreign key is ON DELETE SET NULL, so deleting the account
turns the ticket back into a guest ticket with no save to withdraw the token
on. The secret already sent would then open it.
Only a guest ticket's link carries the token now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 9fe18a -
fix(security): harden how the token parameter is read (#79)
Form::getTicketToken() cast the raw request parameter to string, so a request
carrying token[]=x raised "Array to string conversion" and put the word Array in
the hidden field. The value is only ever a string; anything else is no token.
TicketToken::matches() also trimmed the stored value. Nothing can store
whitespace in that column, and trimming it suggested otherwise — worse, it meant
a column holding only spaces would have compared equal to a request value of
spaces had the emptiness guard not caught it first. Only the request value is
trimmed now, which is the one that travels through a mail client.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 7401ea -
fix(url): scope the helpdesk front name to the ticket's store (#79)
TicketUrl built the path from Helper\Config::getFrontName(), which read the
config at whatever scope the request happened to be in, and paired it with
$store->getUrl() for the ticket's store. A notification is usually sent from
cron or from the backend, where those two are not the same shop, so a store view
with its own front name got a link into the wrong path.
getFrontName() and getAlternativeFrontName() now take an optional store, which
leaves every existing caller alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> f13200 -
perf(setup): fill the backfilled tokens one statement per batch (#79)
Magento wraps a data patch in a single transaction, so a statement per ticket
held an undo log the size of the whole table open for the length of the patch,
and the row locks with it. On a shop with years of tickets that is the kind of
upgrade that times out halfway.
One CASE per batch of 500 keeps the same per-row random value at 1/500th the
statements. Ids are cast to int on the way in and the tokens go through the
adapter's quote(), so the expression cannot carry anything but what it says.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 354756 -
fix(archive): keep the ticket token out of the archive file (#79)
TicketArchivate::write() serialises the whole ticket row into the All Data column
of var/.../swissup_helpdesk_ticket_archive.csv. base64-encoded JSON is encoding,
not encryption, so archiving a ticket wrote a live access credential to a file on
disk in trivially reversible form — a file that gets picked up by backups and
copied into staging dumps.
Nothing needs it there. Unarchiving clears ticket_id and inserts a new row, so
_beforeSave mints a fresh token for the restored ticket on its own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> f35f0f -
Merge branch 'master' into claude/guest-ticket-token-79-d237f7 d04b65
-
Merge branch 'master' into fix/attachment-link-lifetime-91 2e2bfb
-
Merge pull request #105 from swissup/claude/youthful-moser-f78a75
fix(test): type the nullable parameter explicitly for PHP 8.4 (#104) 2df7ca -
fix(test): type the nullable parameter explicitly for PHP 8.4 (#104)
`execute()` declared `array $uploadResult = null`, which PHP 8.4 treats as
an implicitly nullable parameter and deprecates. The deprecation is raised
while PHPUnit is still loading the test file, so the suite aborts with an
"Internal server error" before a single test runs, and the PHP Linter check
fails too. Both have been red on master since a4bc727, and every branch cut
from master inherits the failure.
A sweep of the module turned up no other implicitly nullable parameter; the
remaining `$param = null` defaults are on untyped parameters, which stay
valid.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 0923b6 -
fix(security): withdraw a ticket's token when it is moved to another address (#79)
`email` is in TicketDataFilter::ALLOWED_FRONTEND_FIELDS, which filterAdminData
merges, so an administrator saving a ticket can change the address a guest ticket
belongs to.
Until the token existed that revoked the previous reader on its own: the e-mail
check reads the address off the ticket, so once it changed, the old one stopped
matching. The token is held by the reader instead of re-derived from the row, so
nothing withdrew it and a link mailed to the previous address kept working for
good. Rotating on the change restores what the e-mail check gave for free, and it
is the lever the stored column was chosen for.
The address is compared the way TicketAccess compares it — trimmed, without
regard to case — so that re-saving a ticket without touching the field does not
invalidate links that are already out.
customer_id needs no equivalent: once it is set, TicketAccess takes the customer
branch and never reaches the token at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 8fd484 -
feat(email): put the ticket token in customer notification links (#79)
Both customer templates built the link themselves out of {{store url}} and the
ticket number, which left nowhere to put the token and hardcoded the helpdesk
front name that Helper\Config lets a shop change. The observers now pass a
finished ticket_url and the templates print it.
A ticket with no token yet — one created before the backfill patch has run —
yields the same number-only URL the templates produced before, so nothing
regresses while an upgrade is in flight.
A shop that has customised either template in Marketing > Email Templates keeps
its stored copy and its old markup, so its links stay number-only until the
template is updated. Those links still work: Ticket/View falls back to the
e-mail prompt exactly as it does today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 868444 -
feat(security): let a ticket token stand in for the guest e-mail prompt (#79)
A guest following a notification link in a fresh session is asked for the address
the ticket was created with, which is the thing the link was supposed to spare
them. The token is what the link can carry instead.
The token is a bearer credential and is treated as one: holding it is the whole
proof, not a second factor on top of the e-mail. It is sent to the address that
owns the ticket, which is exactly what the session e-mail check was a weak stand
in for, and unlike the attachment signature in #91 it is per ticket and stored,
so a single ticket can be given a new one without touching anything else.
Two limits on what it buys:
A token never unlocks a ticket that belongs to a customer account. Those stay
behind the customer session, so a forwarded link cannot walk past the login that
guards them today.
A token accepted by Ticket/View does not seed the guest e-mail into the session.
That session value opens every guest ticket opened with the same address, while
the token speaks for the one ticket it was issued for; the reply form carries the
token back instead, so a reply works in a session that never saw the address.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> e400a6 -
feat(security): give each ticket an unguessable access token (#79)
The ticket number is the only identifier a guest ticket has, and
Model/ResourceModel/Ticket builds it from three random lowercase letters over a
sequential id: 17576 candidates per id. Since 1.4.10 that is no longer an access
hole — TicketAccess makes a guest present the ticket's e-mail — but the number
still travels in URLs, in notification e-mails and into every log that records
them, and anything built on top of it inherits the weakness.
The token is a stored column rather than an HMAC over the ticket id, as
AttachmentSignature derives its signature. An HMAC needs no column and no
backfill, but it cannot be withdrawn from a single ticket: the only lever is the
installation crypt key, and AttachmentSignature deliberately accepts every key in
a rotation so that links already sent keep working — so rotating it would not
revoke anything either. A stored value can be replaced one row at a time, is
independent of the crypt key, and leaves room for token-only URLs later without
putting the sequential id back on display.
Existing tickets get a token from the data patch during setup:upgrade, so no
ticket is left without one. The column is nullable so the schema change does not
have to invent values for rows that already exist, and MySQL permits the repeated
NULLs under the unique index until the patch fills them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 7aab75 -
Merge pull request #101 from swissup/claude/docs-inbound-mail-78-2cbaa9
docs(email): state the SPF, DKIM and DMARC requirement for inbound mail (#78) fb3b5b -
feat(security): add an opt-in lifetime for signed attachment links (#91)
A signature is the fallback the download falls back to when nobody is signed in
to the ticket, so it is still a bearer token for that case. Shops that want
those links to stop working after a while can now say how long they last, in
days, under Helpdesk > Security Settings.
The default is 0, no expiry, and signing under it produces exactly the payload
1.4.10 produced: the links already sitting in customers' mailboxes keep
validating, so upgrading changes nothing in the field.
When a lifetime is set, the moment the link dies is part of what is signed and
is carried in front of the MAC, separated by a ":" a hex MAC cannot contain, so
it cannot be moved by whoever holds the URL. Turning the setting on also stops
the links signed without an expiry from being accepted: a link with no expiry in
it can never be aged out, and retiring those is the point of turning it on. The
admin comment on the field says so.
A link that carries an expiry is still honoured, and still expires, after the
setting is turned off again, so flipping it back does not resurrect anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ff0d0c -
fix(file): check the visitor against the ticket before serving an attachment (#91)
The download was gated on the signature alone. A valid signature is a bearer
token: it never expires, cannot be revoked and says nothing about who is
presenting it, so a forwarded notification e-mail, a shared mailbox, a proxy
log or a referrer header hands the file to whoever ends up holding the URL.
Ask TicketAccess about the ticket the message belongs to first. The customer
the ticket belongs to, and a guest whose session carries the ticket's e-mail,
are served whether or not the request has a signature at all.
That branch is limited to messages the visitor is shown. Internal notes are
filtered out of the frontend listing, and the stored path is the uploader's
dispersion of the original file name, so without the limit owning the ticket
would be enough to pull a note's attachment out of it by guessing a message id
and a file name.
The signature stays as the fallback for the case it exists to serve: a request
with no session that knows the ticket — a link opened from a notification
e-mail, and the attachment preview in the backend, which builds frontend URLs
and reaches this controller without a customer session. Internal notes are
still previewable there, as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> dd009b -
docs(email): state the SPF, DKIM and DMARC requirement for inbound mail (#78)
The checks added in 1.4.10 (#70) all read the address in the From: header,
and SMTP does not authenticate it. Nothing in PHP can: the module sees the
message long after delivery. A forged sender belonging to a user of the
department owning a ticket is still stored as a staff reply, and a new
ticket is still bound to the customer account owning the sender address.
Document the mail-server side of that in README.md, which is the only
documentation this repository carries; the fuller setup page lives in the
docs site. Add a notice on the Email Storage form's Host field so the
requirement is visible where the mailbox is configured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ead4ad -
Merge pull request #100 from swissup/fix/admin-upload-payload-99
fix(upload): refuse a malformed admin upload payload with the JSON error (#99) 2cd0af -
Merge pull request #98 from swissup/fix/review-follow-ups-94
fix: review follow-ups — header escaping, audit logging, strict compare (#94) 37578f -
Merge pull request #97 from swissup/fix/uploader-error-escaping-93
fix(upload): set the uploader's error text instead of interpolating it (#93) e4b7e2 -
Merge pull request #96 from swissup/fix/upload-payload-shape-92
fix(upload): answer a malformed file payload with the JSON refusal (#92) 1bf272 -
Merge pull request #95 from swissup/fix/attachment-signature-key-90
fix(file): refuse to sign attachment links without a crypt key (#90) 72401a -
Merge pull request #83 from swissup/fix/low-findings-74
fix(security): close the four low-severity audit findings (#74) 6185f6 -
fix(upload): refuse a malformed admin upload payload with the JSON error (#99)
The admin file upload read two array indexes without checking either, and
handed whatever it found to Model\Upload. A POST without the
swissup_helpdesk_ticket_message[file] part passed null; a field named
...[file][] passed the array-shaped $_FILES entry, where tmp_name and name
are themselves arrays. pathinfo() then raised a TypeError, which extends
\Error and so escaped the catch, and the uploader in the ticket form got an
exception page where it reads {error, errorcode}.
Catching \Throwable would have swallowed the crash instead of refusing the
input, so the shape is checked up front and anything else answers with the
error shape the form already handles.
The refusal is a 200, not a 4xx: Magento_Ui's file-uploader reads the message
off the done handler, and a 4xx reaches a fail handler that only logs to the
console.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> a4bc72 -
fix(file): state the attachment filename as RFC 6266 (#94)
The name went into Content-Disposition between double quotes, and
FileInfo::isSafeFilename() rejects only NUL and a backslash. What keeps a
quote out today is Magento's Uploader with setAllowRenameFiles(true), an
upstream guarantee this line neither states nor checks.
filename*=UTF-8'' leaves no quoting to get wrong and carries a non-ASCII
name correctly, which the quoted form never did.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> c95c9b -
fix(file): compare the preview MIME type strictly (#94)
Both sides are strings today, so it behaves. A comparison that decides
whether a response is rendered inline in the shop's own origin should not
depend on that staying true.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 95b4cf -
fix(message): report a posted is_backend_authored flag (#94)
The allow-list already drops it, so there is no mass-assignment hole. But
the forbidden list is what feeds logMassAssignmentAttempt(), and the field
that decides whether a message's text goes through the CMS filter is the one
an attempt to set is most worth seeing in the log.
Both administrator paths set the flag after the check, on filtered data, so
a legitimate backend save still reports nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 36e1b0 -
fix(upload): set the uploader's error text instead of interpolating it (#93)
addError() built an HTML string around its argument, and since #81 one
caller passes jqXHR.responseJSON.message straight off the wire. Every
message that reaches it today is written in this module, so nothing is
exploitable as the code stands, but the sink is one third-party validator
message away from echoing something a visitor chose.
Binding the click handler to '.swissup-attachment-error' also re-bound it to
every error already on the page, so the handlers accumulated with each new
one. The node that was just built is the only one that needs it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> df6fb6 -
fix(upload): answer a malformed file payload with the JSON refusal (#92)
is_array() passes the array-shaped $_FILES entry a field named fileupload[]
produces, where tmp_name and name are themselves arrays. pathinfo() then
raises a TypeError, which is not a LocalizedException and so escapes the
catch: the ajax caller is handed an HTML exception page instead of the
refusal it knows how to display. A payload whose upload failed arrives the
same way, with an empty tmp_name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ca5188 -
fix(file): refuse to sign attachment links without a crypt key (#90)
An HMAC keyed with the empty string is not a secret. Anyone who knows the
message id and the stored file path could recompute the signature and
isValid() would accept it, which is the defect #73 was filed for, reached
by a different route and without a word in the log.
An installation with no readable crypt/key is broken, not an installation
that should go on handing out download links. Both signing and validation
now stop there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> af9c9e -
fix(upload): stop the new log line from being able to fail an upload (#74)
`Mime::getMimeType()` throws `FileSystemException` when the file is not there,
and it was called only to decorate a notice — on the one path that previously
never touched the file at all. A missing tmp file turned an upload that used to
proceed into a refusal, for the sake of a log field. It is sniffed in a try now
and reported as "undetermined" when it cannot be read.
The `$fileName` parameter loses its default; the method is private and its only
caller always passes one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> d044a6 -
fix(security): let an address block bite again after the re-keying (#74)
Re-keying the counters on the visitor left `blockIp('1.2.3.4')` writing a record
that nothing would ever read: `checkLimit()` resolves to `customer:` or
`session:` for anyone carrying a session, so the address record was never
consulted. A public method that silently does nothing is worse than no method.
`checkLimit()` now tests the address alongside the visitor, and an identifier
that carries none of the known prefixes is read as the bare address an operator
means. Automatic blocking is unchanged and still only ever lands on the visitor,
so a shared proxy address cannot collect somebody else's violations.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 8e3397 -
fix(cron): make the per-message guard actually contain a failed message (#74)
Two holes in the containment this file claimed to have.
`catch (\Exception)` does not catch `\Error`, so a TypeError anywhere in the
body still left execute() and took every remaining department with it — the
exact failure this guard exists to stop, and not hypothetical: a TypeError on
the upload path was found in this module this week. Both catches now take
`\Throwable`.
Skipping the removal sweep for a department that had one failure also kept the
messages that succeeded. POP3 carries no flags, so the FLAG_SEEN check never
fires there and every survivor was imported again on the next run; the duplicate
check only holds for a message that carries a Message-Id. A mailbox stuck on one
bad message therefore duplicated the rest, every cron tick. Successful message
numbers are collected instead and removed in descending order, so only the
message that failed is left behind.
The numbers are collected in `finally`, because the body leaves the iteration
with `continue` for a message that was seen, blacklisted or already imported.
Those are handled too and must still be removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 21285c -
Merge remote-tracking branch 'origin/master' into fix/low-findings-74 0df144
-
Merge pull request #89 from swissup/fix/autoclose-system-change-88
fix(notify): stop reporting auto-close as a customer update (#88) cfd4fd -
fix(notify): stop reporting auto-close as a customer update (#88)
The auto-close cron saves tickets with a new status, no message data and
no backend session. _afterSave() therefore wrote the status-change
message with user_id NULL, and AdminNotification treats any message
without an admin user as the customer's. Every auto-closed ticket sent
the department a "customer updated the ticket" e-mail.
A caller can now mark a change as made by the module itself with
is_system_change in the swissup_helpdesk_ticket_message data.
_afterSave() copies the mark onto the new message. The mark is not a
column, so it only exists on the instance the save_after observers
receive. AutoClose sets the mark and states user_id => null. Since #86
a stated user_id wins over the backend session, so a cron run from a
backend request is not credited to whoever triggered it.
AdminNotification only reports messages that are neither from an admin
nor a system change.
AutoClose is the only caller that saves a ticket with neither message
data nor a backend session. The module has no webapi.xml or GraphQL
schema. The frontend controllers are real customer actions, and their
message data is whitelisted to text and file, so a customer cannot set
the mark.
The department-user loop also skipped the user whose id matched
$ticketMessage->getUserId(). That code only runs for messages without
an admin user, where getUserId() is 0, and it compared int to string
strictly. It could never match, so it is removed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> fa761f -
Merge pull request #87 from swissup/fix/staff-email-replies-86
fix(mail): state who wrote picked-up mail; keep staff e-mail replies internal by default (#86) 3e2e52 -
fix(security): hash the session id before it becomes a rate-limit identifier (#74)
The identifier is written to the log on every violation and every block, so
keying anonymous visitors on the raw session id put a live session cookie in
var/log — a credential, not an attribute. Hashing it at the point it is resolved
keeps the counter working and leaves nothing reusable in the log. Customer ids
and addresses stay as they are; neither is a secret and both are what an
operator needs to read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 3d2a4c -
Merge remote-tracking branch 'origin/master' into fix/low-findings-74 956abb
-
fix(mail): state who wrote picked-up mail; keep staff e-mail replies internal by default (#86)
The e-mail pickup put the staff member's id on the message, but
Ticket::_afterSave() built the row with the backend session's user
instead. Under cron that is nobody, so a staff reply by e-mail was stored
as a customer message: the customer got nothing and the department was
told about a "customer update". Run from the Pick Up button, every
message of the run, customers' included, was attributed to the admin
who clicked, and a customer's own mail went back to them as a staff
reply.
_afterSave() now takes user_id from the message data when the caller
states it, null included, and falls back to the session only when it is
absent (mass status/priority actions). The pickup always states it, and
the admin ticket form states the logged-in admin, overwriting any posted
value.
The staff check rests on the From: address alone, which is not
authenticated (#78). A new setting, Mail Settings > "Send staff e-mail
replies to the customer", decides what a staff reply becomes:
- No (default): an internal note by that staff member; the ticket
status is left alone and nothing is mailed;
- Yes: a public staff reply, status Replied, mailed to the customer.
Only safe when the MTA rejects mail failing SPF/DKIM/DMARC.
A staff address that opens a new ticket is that ticket's requester, as
before. The pickup also took the id of the department-user link row
instead of the admin's user_id; it now uses user_id.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 204b25 -
Merge pull request #85 from swissup/fix/inbound-message-id-84
fix(mail): keep inbound Message-Ids and make threading lookups match (#84) de1965 -
fix(mail): keep inbound Message-Ids and make threading lookups match (#84)
Three faults on one path, each enough to break the pickup's threading and
duplicate check:
- ResourceModel\Ticket::_afterSave() dropped the Message-Id the pickup hands
it, so inbound ids were never stored and PickUp's duplicate check could not
match. The notification observers then overwrote the column with the id of
the mail the store sent.
- Those outgoing ids were stored as "<id@host>", while the parser returns
In-Reply-To as "id@host". A reply to a notification was never found by
In-Reply-To either; threading worked only through the subject's ticket
number.
- loadByEmailMessageId('') matched the first row with an empty id, which is
most rows. A new e-mail with no In-Reply-To was resolved to that message's
ticket: rejected with a misleading ownership warning since #70, or appended
to it when the sender owned it or was staff. An e-mail with no Message-ID
was taken for a duplicate and deleted from the mailbox unread.
Ids are now stored and looked up without angle brackets, an empty id never
queries, _afterSave() keeps the inbound id for anything not written in the
backend, and the observers no longer overwrite an id a message already has.
A data patch strips the brackets from existing rows.
The inbound id is attacker-supplied and is echoed into In-Reply-To and
References of outgoing mail, so anything that is not a plain printable msg-id,
or exceeds the 255-character column (an insert error in strict mode), is
dropped rather than stored.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 1a3961 -
Merge pull request #82 from swissup/fix/download-hash-and-preview-73
fix(file): sign attachment links with a real secret and stop previewing documents (#73) 25f292 -
Merge pull request #81 from swissup/fix/upload-endpoint-guards-72
fix(upload): require POST, a form key and the rate limit on the upload endpoint (#72) 321f9b -
Merge pull request #80 from swissup/fix/cms-filter-stored-xss-71
fix(message): run only backend-authored text through the CMS filter (#71) 286fb3 -
fix(file): keep PDF preview out of the CSP sandbox, drop unused encryptor (#73)
The bare sandbox directive puts the inline response in an opaque origin.
Browser PDF viewers have fallen back to downloading the file in that case,
which would turn the admin's PDF preview into a download. PDF script runs in
the viewer's own isolate, never in the shop's origin, so the sandbox buys
nothing there; images and text/plain keep the full policy, and every inline
response keeps nosniff.
FileInfo no longer hashes anything since links are signed by
AttachmentSignature; its EncryptorInterface argument was left unused. The
constructor already changed in this branch, so dropping it costs no extra
compatibility.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 638b64 -
fix(upload): upload one file at a time and stop once refused (#72)
Selecting thirty files is one click, and the widget fired one request per file
in parallel. With file_upload now metered, the first twenty are accepted and
every request after that counts as a rate-limit violation; ten violations block
the address for an hour, for ticket and message creation as well. A legitimate
bulk attachment locked the visitor out of the helpdesk entirely.
Chain the uploads and abandon the rest after the first server refusal, so a
bulk selection trips the limit once rather than a dozen times. A file rejected
by the client-side extension or size check still lets the others through.
The endpoint also reached Model\Upload with a null payload when a POST carried
a valid form key but no file, which is a TypeError and an HTML exception page
for an ajax call rather than the JSON refusal the uploader can show.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 09f477 -
fix(message): record backend authorship instead of inferring it (#71)
isTrustedAuthor() read user_id and an empty email_message_id off the stored
row. Neither says who wrote the text.
The Pick Up button on the ticket grid runs Cron\EmailStorage\PickUp inside the
administrator's own backend request, so ResourceModel\Ticket::_afterSave()
stamped that administrator's user_id onto every inbound message of the run.
_afterSave() never copies the inbound Message-Id, so email_message_id stayed
empty too: an e-mail body reached the CMS filter, raw HTML and {{...}}
directives included, which is the hole #71 is about. Adminhtml\TicketMessage\
AjaxSave gave the same pair to any existing message an administrator re-saved,
including a customer's.
In the other direction the notification observers write the outgoing mail's
Message-Id onto the message they report, so an administrator's own reply lost
its authorship the moment the customer notification went out, and the option
silently stopped working for the one case it exists for.
Record the fact instead. swissup_helpdesk_ticket_message.is_backend_authored is
set by the two paths that know the text came from a backend form -- the admin
ticket Save controller and AjaxSave creating a new message -- and read by the
policy. _afterSave() takes it from its caller rather than from the session.
Existing rows default to 0, so messages written before the upgrade render
escaped. No backfill: user_id plus an empty email_message_id is exactly the
inference this commit removes, and the option is off by default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 284e11 -
fix(security): key rate limiting on the visitor, not the remote address (#74)
Every counter was keyed on RemoteAddress::getRemoteAddress(). Behind a proxy or
CDN — the default Magento setup, where remote_addr_headers is empty — that is one
address for the whole shop, so ten violations from a single abuser tripped
BLOCK_THRESHOLD and locked the helpdesk for every visitor for an hour. Configure
remote_addr_headers instead and the value comes from a client-supplied header,
which makes the limit bypassable by rotating it.
Counters are now keyed on the customer id, falling back to the session id, and
only to the remote address when there is no session at all. A shared address no
longer collects other people's violations, and a forwarded header no longer
resets anyone's bucket.
The trade-off: a guest who discards the session cookie between submissions starts
a fresh bucket, which an address-keyed counter would have caught on a shop with
direct connections. That case belongs to the CAPTCHA the module already wires
into both forms, not to a counter that can lock out real customers.
blockIp()/unblockIp()/isBlocked() keep their names — they are public API — but
now act on the resolved identifier, and the log context and the visitor-facing
message no longer say "IP address".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 541e3a -
fix(cron): keep e-mail pickup running when one message or mailbox fails (#74)
The try wrapped the whole loop over a mailbox and its catch did `return false`,
leaving execute() entirely. One message that threw — bad encoding, a missing
header, a failing save — therefore aborted the department it was in and every
department queued after it, on every cron run, until someone removed it by hand.
The catch is now per message, and fetching the message happens inside it, since
that is where a malformed message throws. Opening the mailbox is wrapped
separately so an unreachable IMAP host skips only its own department.
Messages are deleted in one sweep after the loop, so a department that failed on
any message keeps its whole mailbox and logs why. Catching and continuing without
that would have deleted the message that was never processed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 7aa1b7 -
fix(upload): log the attachments that receive no content inspection (#74)
validateFileContent() returns early for any extension outside KNOWN_MIME_TYPES,
so an extension the admin adds to the allow-list — xsl, csv, odt — reaches disk
with nothing but the extension whitelist behind it. The doc block already told
the reader a notice was logged in that case; nothing was.
The early return stays. Refusing unknown extensions outright would break every
installation that extended the list on purpose, and there is no execution path
to abuse today: attachments are written under var/, outside the document root,
and are served by Controller/TicketMessage/File with nosniff and a download
disposition. What was missing is the operator's ability to see it happening, so
the notice carries the file, the extension and the MIME that was sniffed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 813258 -
fix(admin): require the ticket ACL resource on the ticket grid controller (#74)
Grid declared neither ADMIN_RESOURCE nor _isAllowed(), so Magento fell back to
Magento_Backend::admin and any logged-in admin user could pull the ticket
listing, including the customer-filtered view, without the Swissup_Helpdesk::ticket
permission. Every other controller in Controller/Adminhtml declares one.
The unused Swissup\Askit import went with it; Askit is not a dependency of this
module and the symbol was never referenced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 367d03 -
fix(file): sign attachment links with a real secret and stop previewing documents (#73)
The download link's hash was the sha256 of the message's created_at, with no
salt and no key. validateHash() takes the salt out of the hash it is handed, so
the timestamp was the whole secret — and it is printed next to every message as
"x minutes ago". Anyone who knew the message id, the stored path and roughly
when it was written could produce an accepted link, and the hash was the only
authorisation the action had.
Links are now signed with an HMAC over the message id and the file path, keyed
with the installation's crypt key. Every configured key is accepted on
validation and the newest one signs, so rotating the key does not break links
already sent out.
Inline preview no longer serves text/html or image/svg+xml. From the shop's own
origin those are documents that run script in the visitor's session, not
pictures; they are downloaded instead. What is still previewed is sent with
nosniff and a restrictive Content-Security-Policy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ccccd9 -
fix(upload): require POST, a form key and the rate limit on the upload endpoint (#72)
The endpoint wrote a file to var/helpdesk with no checks at all: it ran on GET,
validated no form key, and never called the rate limiter, although a file_upload
bucket had been declared for it since the limiter was added. An anonymous client
could fill the volume in a loop, and the endpoint was reachable cross-site.
It now refuses anything but POST, validates the form key, applies the declared
limit, and — like the ticket and message controllers — only serves guests where
guests may open a ticket at all. Refusals come back as JSON with a 400 so the
uploader can show them, instead of an exception page for an ajax call.
The uploader's error branch was registered as `fail`, which jQuery.ajax never
calls, and used `this` where the widget was meant; it now reports the server's
message.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 5fd3ca -
fix(message): run only backend-authored text through the CMS filter (#71)
The CMS page filter renders raw HTML and executes {{...}} directives. Every
message went through it when the option was on, including text written by
customers, guests and inbound e-mail, which is a stored XSS path against the
customer and against any agent opening the ticket.
Authorship now decides, not the option alone: text an administrator wrote in
the backend is still rendered as rich content, everything else is escaped.
An administrator's address on an incoming e-mail does not count, because
From: is not authenticated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 11a308
-
-
1.4.10
-
Version 1.4.10 f95e5c
-
Merge pull request #76 from swissup/fix/inbound-email-sender-70
fix(cron): do not append inbound e-mail to a ticket the sender does not own (#70) 62b851 -
Merge pull request #75 from swissup/fix/ticket-reply-ownership-69
fix(message): reject replies to tickets the visitor does not own (#69) e35a26 -
Merge remote-tracking branch 'composer/master' into fix/ticket-reply-ownership-69
# Conflicts:
# Controller/TicketMessage/Save.php 5b8a62 -
Merge pull request #77 from swissup/chore/drop-non-english-comments
style: drop the Ukrainian and emoji comments from the security hardening d428a9 -
fix(cron): guard the remaining header accessors and the blacklist (#70)
Review of this branch found the "no longer throws on a missing From:" claim was
wrong, and that the blacklist had the same first-address weakness the ownership
check had just been fixed for.
`getFromAddresses()` was guarded, but `getFrom()` was not, and `PickUp` calls it
one line earlier — so a message with no From: still threw out of `execute()` and
still took pick-up for every remaining department with it (#74). `getSubject()`
had the same shape and is reached for the ticket number and the new ticket's
title, so a message with no Subject: did the same. Both return an empty string
now, and `getFromName()` goes through `getFrom()` rather than reading the header
itself.
The blacklist only ever saw the first address, so `From: clean@example.com,
blacklisted@evil.test` walked past it while the message was delivered from the
blacklisted mailbox. Every address in the header is checked now.
`getFromAddresses()` caches its result and `setMessage()` clears it, so the
header is parsed once per message instead of three times.
The test for a header-less message now covers every accessor the cron calls on
the parser, which is what would have caught `getFrom()`: it fails on the
previous commit with "Header with Name from or from not found" at Parser.php:243.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 175135 -
fix(i18n): add the ticket-not-available message to en_US.csv (#69)
The string this branch adds to `Controller/TicketMessage/Save` was the only
user-facing one in that controller missing from the translation file, so it
stayed English on every other locale.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 7cdd8c -
style: keep the single-line @var hints and the ResourceModel note
Review of this branch found the cleanup took three things it should not have.
The doc block tidier matched `/** ... */` with DOTALL, so a block that opens
and closes on one line looked like a block whose content had all been removed
and was dropped whole, whatever language it was in. That deleted all nine
`/** @var ... */` type hints in the controllers and the observer — the ones
PHPStan and the IDE need on `resultFactory->create()` and
`$transport->getMessage()`. The tidier now leaves single-line blocks alone.
`TicketDataFilter::ADMIN_ADDITIONAL_FIELDS` loses its Ukrainian notes but keeps
the fact they carried, in English: those four keys are consumed by
`ResourceModel\Ticket::_beforeSave()`/`_afterSave()`, and dropping them from the
whitelist silently discards the data before it reaches them. That is not
visible from this file.
`// varchar(128)` and `// primary key` are gone too. They had survived only
because they happened to be English while every neighbouring entry lost its
note, which read as arbitrary.
Still comment-only: with comments and whitespace removed, the token stream of
all 12 files is identical to master.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 0e443b -
style: drop the Ukrainian and emoji comments from the security hardening
The October hardening wave left its working notes in the code — Ukrainian
prose and ✅/⚠️ markers in `// ЗАХИСТ:` comments across the controllers, the
Service filters, the mail classes and the data patch. 120 comment lines are
removed and 61 more lose a Ukrainian trailing note or description.
Doc block tags keep their type and variable: a `@param array $fields` whose
only description was Ukrainian becomes `@param array $fields` rather than
disappearing, so no API documentation is lost. A block left with nothing but
tags keeps the leading `*` line this module already uses elsewhere.
Comment-only: with comments and whitespace removed, the token stream of all
12 files is byte-identical to master.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> fbc509 -
style(security): drop the explanatory comments from the access check (#69)
The reasoning stays in the commit messages and the pull request.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 9e649d -
style(security): drop the explanatory comments from the sender check (#70)
The reasoning stays in the commit messages and the pull request.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 94e19b -
fix(security): take the sender from the parsed From:, scope staff to the department (#70)
Review of this branch found the new check could be walked around, and that it
left the staff half of the finding open.
The sender was read with `preg_match('|<(.*?)>|')` over the raw From: header —
the first bracketed address wins. RFC 5322 allows more than one mailbox in
From:, and a display name may itself contain angle brackets, so either of
From: Owner <owner@example.com>, Attacker <attacker@evil.test>
From: "x <owner@example.com>" <attacker@evil.test>
made `getFromEmail()` return the victim's address while the message was
delivered from the attacker's, and the new ownership check then let it into the
victim's ticket. Verified against laminas-mail: both headers yield
`owner@example.com` from the old code.
`getFromAddresses()` now asks Laminas for the parsed address list and reports
every address it holds, so a header naming several mailboxes — or a header sent
twice — stays visible as more than one sender instead of collapsing to the
first. `hasSingleFromAddress()` is what the cron holds the message against: with
no single sender the message reaches no existing ticket, is never attributed to
staff, and opens a ticket of its own without being bound to a customer account.
`getFromEmail()` keeps its contract for the blacklist and the staff lookup, and
no longer throws when the header is absent, which used to abort pick-up for
every remaining department.
Staff status is no longer a blanket pass. It is decided against the users of the
department whose mailbox received the message, which says nothing about a ticket
belonging to another department, so `isAllowedOnTicket()` now also requires the
ticket to be that department's. Without this, any address matching any
department user reached every ticket in the shop and was stored as a staff
reply — finding 2 of the issue. The remaining exposure is an unauthenticated
From:, which needs SPF/DKIM at the MTA.
The new ticket opened for a rejected message now records in its notes which
ticket the message had referenced, so the two can be linked from the grid
rather than only from the log.
Parser tests skip where laminas/laminas-mail is not installed, as it is an
optional dependency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 157509 -
fix(security): tighten the ticket access check after review (#69)
Three points from the review of this branch:
* The guest e-mail is now taken from the session only. The reply form never
posts an `email` field, so accepting one from the request added a way to
reply without ever passing through `Ticket/View`, where the address is
checked and remembered. Nothing legitimate used it.
* E-mail addresses are compared with `strcasecmp` instead of `===`. The
ticket keeps the address exactly as it was typed into the form, so an
owner coming back with a differently cased spelling was being refused.
This also makes the rule the same as the one `InboundMailSender` applies
to incoming mail.
* A non-string e-mail is treated as no e-mail rather than cast. `email[]=x`
arrives as an array, and `(string) $array` raises "Array to string
conversion", which Magento turns into an exception in developer mode — a
500 instead of a refusal.
The unit tests now set `customer_id` and `email` as data and let the real
Ticket accessors run, instead of mocking them. `getCustomerId()` casting to
int is what makes the strict comparison correct, and the previous mocks
would have stayed green without it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 3904ce -
fix(cron): do not append inbound e-mail to a ticket the sender does not own (#70)
Pick-up resolved the ticket from In-Reply-To or from the number in the
subject and appended the message without looking at the sender, so a
forged From: could post into another customer's ticket — as staff, if
the address matched a department user.
The sender now has to belong to the ticket. When it does not, the
reference is dropped and the message opens its own ticket, with a
warning in the log: nothing is lost and nothing lands in a conversation
it does not belong to.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 10123f -
feat(security): add sender check for inbound e-mail (#70)
Answers whether an unauthenticated From: may be appended to an existing
ticket: staff of the receiving department, the address the ticket was
opened with, or the account it belongs to. Covered by unit tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 22fee1 -
fix(message): reject replies to tickets the visitor does not own (#69)
The reply controller loaded the ticket by its public number and appended
the message without ever tying the ticket to the session, so any visitor
holding a valid form key could post into another customer's ticket.
The ticket now has to pass the same owner check the view controller
applies, and an unknown ticket and a foreign one give the same answer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 9428a3 -
refactor(ticket): take the view controller's owner check from the shared service (#69)
No behaviour change: the same two rules, now expressed once so the view
and the reply controller cannot drift apart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 06c155 -
feat(security): add ticket access checker shared by view and reply (#69)
A single place that answers "may this visitor act on this ticket":
a customer ticket belongs to its customer, a guest ticket to whoever
presents the e-mail it was created with. Covered by unit tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 057646
-
-
head
-
Fix tabs.js cleanup on destroy, selector escaping and scroll button clicks 9cd5d6
-
Fix double-encoded widget_identifier when installers update tabs cc0db3
-
Cache empty tabs result to avoid repeated tab queries 7982ca
-
Fix stale full page cache for tabs, ajax tabs and GraphQL
- Tag pages with the generic easytabs_entity tag and flush it on tab save, so new or enabled tabs appear on cached pages
- Add product cache tags to ajax tab responses
- Add CMS block cache tags to GraphQL getEasyTab responses a93cca -
Fix mass Enable/Disable skips widget tabs ef2b21
-
Fix `tabsUpdater._destroy` throws `ReferenceError` 7bae6f
-
Handle malformed URI sequences in tab anchor decoding 973dd6
-
Fix customer validation for guest users by handling missing group_id f06ace
-
Enhance Ajax tab filtering and validation, implement safe parameter handling in LoadOptions and NewConditionHtml, and ensure proper product checks in Index controller ead1aa
-
Fix missing aria-level on tab headings in expanded layout (#39)
* Fix missing aria-level on tab headings in expanded layout
* Fix missing aria-level attribute for expanded tabs on Breeze
---------
Co-authored-by: Vitaliy Dmytruk <vitaliy@swissuplabs.com> 3dab50 -
Fix missing aria-level attribute for expanded tabs on Breeze 05c0a6
-
Fix missing aria-level on tab headings in expanded layout 2523f0
-
-
1.13.6
-
Version 1.13.6 fe61d0
-
Fix product visibility bypass and GraphQL tab conditions bypass (#38)
* Fix product visibility bypass and GraphQL tab conditions bypass
- Helper\Product::canShow(): parent_id fallback now applies only to
enabled products and only when parent_id is their real parent
(catalog_product_relation). Previously any visible product id in
parent_id exposed disabled products via the easytabs AJAX endpoint.
- GraphQL getEasyTab now validates tab conditions (customer group, etc.)
using the customer group from GraphQL context, like the storefront does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Address review: stable parent check, duplicate aliases, real customer group
- Helper\Product: check parent's children via type instance
getChildrenIds() instead of Relation::getRelationsByChildren(),
whose return shape is not a stable API. Drops ObjectManager usage.
- GraphQL data provider: with duplicate aliases, return first tab that
passes conditions instead of failing on the highest-priority one.
- GraphQL resolver: load caller's customer group via customer repository
instead of defaulting to guest when context has no customer_group_id
(guest fallback exposed "not in group X" tabs on older Magento).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Pick same tab as storefront for duplicate aliases in GraphQL
Mirror Block\Tabs::_buildTabs(): among tabs passing conditions, prefer
the requested store view over default store view (0), and within a store
take the same tab the storefront does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 90330e -
Pick same tab as storefront for duplicate aliases in GraphQL
Mirror Block\Tabs::_buildTabs(): among tabs passing conditions, prefer
the requested store view over default store view (0), and within a store
take the same tab the storefront does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 06c0a8 -
Address review: stable parent check, duplicate aliases, real customer group
- Helper\Product: check parent's children via type instance
getChildrenIds() instead of Relation::getRelationsByChildren(),
whose return shape is not a stable API. Drops ObjectManager usage.
- GraphQL data provider: with duplicate aliases, return first tab that
passes conditions instead of failing on the highest-priority one.
- GraphQL resolver: load caller's customer group via customer repository
instead of defaulting to guest when context has no customer_group_id
(guest fallback exposed "not in group X" tabs on older Magento).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 3871b9 -
Fix product visibility bypass and GraphQL tab conditions bypass
- Helper\Product::canShow(): parent_id fallback now applies only to
enabled products and only when parent_id is their real parent
(catalog_product_relation). Previously any visible product id in
parent_id exposed disabled products via the easytabs AJAX endpoint.
- GraphQL getEasyTab now validates tab conditions (customer group, etc.)
using the customer group from GraphQL context, like the storefront does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 3a8cc0
-
-
head
-
Better dependency resolving inside bundle (checkout tests) cc25ca
-
Added missing methods to checkout components 3a587f
-
Fixed possible js error when calling debug early 689d66
-
Improve runtime nested deps resolving (A > B > C > D) 85a9bd
-
Read deps from requirejs-config when better compat is enabled 8eb715
-
Added missing `$.Event` be2d4d
-
Fixed missing child components at checkout page 37df49
-
Fixed checkout integration errors a45aac
-
uiComponents: Fixed empty provider when dataScope is not set a9dfcf
-
Prepare nested children as well. Fixes checkout flatrate validation 9905d7
-
Do not ignore rjsResolver anymore 7e40af
-
CheckoutLoader component 042f34
-
Align `$.Deferred` with jQuery API c9f87f
-
Align `$.request` with jQuery API 8aa15c
-
Improve `:visible` selector support 91a9ff
-
Fixed undefined child in elems when uiLayout component is not loaded yet 345194
-
Added `repeat` knockout binding (payment methods list) ba27e4
-
Wait for mixins before resolving a dependency e24fd4
-
Custom modal should not render lazily (checkout order summary) 3bfe11
-
Fixed infinite focus loop when validating single field (login popup) 59cd0c
-
Allow using `:visible` hack for multiple selectors 268c1a
-
Fixed mixins not applied to module dependencies when loaded asynchronously 5d91b8
-
Basic implmentation of `ko.datepicker` 05df54
-
Align date with moment.js 8ce8c3
-
Align calendar with jQueryUI f1cd4a
-
Align ui cmp initialization with Magento. Do not pass element.
This fixes `google-pay-cart` logic. d292d3 -
Implement `ko.fadeVisible` 4c14f6
-
Implement simplest version of `rjsResolver` 3440bb
-
Fix possible js error: read source synchronously first a77cb4
-
Align js cmp with Luma: Don't overwrite existing fn with `undefined` baea61
-
Added `validation.check` method 531321
-
`$.grep` implementation c5ec90
-
Fixed possible js error `Cannot read properties of undefined` fb7bef
-
Do not load js component if it uses `componentDisabled: true` cd0baf
-
-
2.32.1
-
Version 2.32.1 c5afe6
-
Prevent infinite loop when head contains invalid stylesheet link 582a77
-
Fixed incorrect magnifier image when using expanded gallery fc2f6b
-
Prevent auth popup form submit when recaptcha is not ready yet
Closes #101 740ada -
Fixed incorrectly marked lazyload when main image is not the first one d5cd6b
-
Do not add dom duplicates to $.registry 84beb6
-
-
1.21.1
-
Version 1.21.1 272eb4
-
Merge pull request #138 from swissup/fix/critical-css-review-followups
fix(critical-css): review follow-ups for #137 (#133) e6d505 -
fix(defer-css): trust the built-in critical path, keep Breeze preloads (#133)
Breeze prints its critical CSS as a plain <style> without
data-type="criticalCss", so the DOM check from the previous commit made
DeferCss skip every Breeze page: stylesheets stayed on Breeze's
media="print" fallback instead of rel="preload" (lowest fetch priority).
Look for the criticalCss node only when dev/css/use_css_critical_path is
off; with it on, Magento or Breeze provides the critical CSS.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> c9ddc5 -
fix(critical-css): keep Default Critical CSS from closing its style tag (#133)
The default CSS is printed raw inside <style>; a "</style" sequence in the
admin value would end the tag early and let the rest render as HTML.
Escape it, add the missing docblock summary flagged by phpcs, and cover
the block with unit tests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 47fb93 -
fix(defer-css): do not defer stylesheets when the page has no critical CSS (#133)
Critical CSS enabled with the built-in path off and an empty Default
Critical CSS still turned every stylesheet into rel="preload", leaving the
page unstyled until they loaded. DeferCss now runs only when a non-empty
<style data-type="criticalCss"> is present in the document.
HTML fragments without critical CSS are no longer deferred either; the
tests now inject a critical style block into their fixtures.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 1f6912 -
Merge pull request #137 from swissup/fix/default-critical-css-without-builtin
fix(critical-css): output Default Critical CSS without built-in critical path (#133) 72ae02 -
fix(critical-css): output Default Critical CSS without built-in critical path (#133)
With "Use built-in critical CSS feature" off, DeferCss still deferred the
stylesheets, but the Default Critical CSS never reached the page: it was
only appended through a plugin on Magento's Header\CriticalCss block, which
Magento renders solely with dev/css/use_css_critical_path. The page got
deferred CSS and no critical CSS.
Up to 1.9.9 the CriticalCss optimizer inserted the default CSS itself;
27ecf46 ("Fix criticalCss dublication", 1.10.0) dropped that path.
Add a head.critical block that renders the default CSS as
<style data-type="criticalCss"> only when critical CSS is enabled for the
page and the built-in path is off, so the CriticalCss optimizer still
minifies it and the built-in path cannot produce a duplicate.
Fixes #133
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> b98e40
-
-
1.21.0
-
Version 1.21.0 28f0bf
-
fix(installer): do not enable Advanced JS Bundling by default 19fc09
-
Merge pull request #134 from swissup/feat/extract-advanced-js-bundling
refactor: move Advanced JS Bundling to swissup/module-advanced-js-bundling 343655 -
fix: keep deprecated config path constants, skip the order test without the new module
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> 9d53ff -
Merge master into feat/extract-advanced-js-bundling
# Conflicts:
# composer.json 1714b2
-
-
1.2.0
-
Added Apple login, see #7 3fedc9
-
-
1.1.13
-
1.1.0
-
Added Apple login, see swissup/module-social-login#7 63067b
-